Skip to content

More Data From the PDF#488

Open
TomerMe2 wants to merge 28 commits into
masterfrom
read_pdf_tbl_3
Open

More Data From the PDF#488
TomerMe2 wants to merge 28 commits into
masterfrom
read_pdf_tbl_3

Conversation

@TomerMe2

@TomerMe2 TomerMe2 commented Dec 27, 2021

Copy link
Copy Markdown
Collaborator

This PR brings us tables:

  • 1.4
  • 1.6
  • 1.7
  • 1.8.4
  • 3.1
  • 3.2
  • 7.1

This PR also brings an improvement in runtime since it knows to cut the appendixes from a heavy PDF.

@TomerMe2
TomerMe2 requested a review from GalGend January 15, 2022 09:29
Comment thread server/api/lib/mavat/index.js Outdated
Comment thread server/api/lib/mavat/planInstructions/index.js Outdated
Comment thread server/api/lib/mavat/planInstructions/index.js Outdated
Comment thread server/api/lib/mavat/planInstructions/index.js Outdated
Comment thread server/api/lib/mavat/planInstructions/index.js Outdated
// this function look for the correct columns for the given headers, and returns a factory embedded with these findings
const rowAbstractFactoryChart1_7 = (firstPageOfTable, headersStartIndex) => {
// clean the headers
firstPageOfTable = firstPageOfTable.map(row => row.map(cell => cell.replace(/\n/g, ' ')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if firstPageOfTable is undefined or not an array?

@TomerMe2 TomerMe2 Jan 24, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always an array. We can add a check beforehand, but we firstPageOfTable comes from the PDF extension and it's consistent.
Adding a check here will require adding the same check on at least 10 places.

contains: getFromArr(row, containsIdx),
scale: getFromArr(row, scaleIdx),
number_of_pages: getFromArr(row, numberOfPagesIdx),
edit_date: getFromArr(row, editDateIdx),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snake casing in purpose?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, for the db. By using snake casing we don't need to change the map's keys when inserting into the db.


if (headersStartIndex === -1) {
// it's an error
log.error('didn\'t find headers for some char 1.6');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add planId to the log so we can assign to a process for investigation?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a planId here requires me to pass planId into more software layers (at least 2 more) and change at least 25 lines in at least 15 different files. It will be a bit complicated.
The crawler that runs in production doesn't log the planId if it fails. Can you check the logs to see if we got a lot of "didn't find headers for *" to see if it worth investing in?


if (headersStartIndex === -1) {
// it's an error
log.error('didn\'t find headers for some chart 3.1_without_change');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plan id

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing as the comment on the log.error on 1.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants