Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: declare pdf whitespaces in global variable to avoid bug #411

Merged
merged 5 commits into from
Apr 26, 2021
Merged

Refactor: declare pdf whitespaces in global variable to avoid bug #411

merged 5 commits into from
Apr 26, 2021

Conversation

LucianoHanna
Copy link
Contributor

Hi,

I made some refactor in RawDataParser, I declared a global variable containing pdf whitespaces so we don't need to hardcode were use whitespace, avoiding bug if forget to consider all pdf whitespaces

Copy link
Collaborator

@k00ni k00ni left a comment

Choose a reason for hiding this comment

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

@LucianoHanna thank you for the PR.

I welcome your changes in general but have to ask you to move $pdfWhitespaces and $pdfWhitespacesRegex to Config.php. Are these the "global" variables you meant in the title?

Its purpose is to hold default values which can be overridden by the user. For some PDFs it may require to change a default value. Here is a basic example how to setup and use Config instance.

Also which bug does this PR avoid?

src/Smalot/PdfParser/RawData/RawDataParser.php Outdated Show resolved Hide resolved
src/Smalot/PdfParser/RawData/RawDataParser.php Outdated Show resolved Hide resolved
src/Smalot/PdfParser/RawData/RawDataParser.php Outdated Show resolved Hide resolved
@k00ni
Copy link
Collaborator

k00ni commented Apr 20, 2021

Another remark: Please create a separate branch for each pull request in the future. It limits our options if you are using master.

@LucianoHanna
Copy link
Contributor Author

@k00ni thank you for your remarks!
This PR is a refactor to avoid future bug if some future commit forget to consider all pdf white spaces (like the one fixed in #405)

I refactored the code to remove hardcoded whitespaces and use a config variable, this way we avoid having some line of code considering only %20 as a white space

@LucianoHanna LucianoHanna requested a review from k00ni April 20, 2021 17:19
src/Smalot/PdfParser/Config.php Outdated Show resolved Hide resolved
@LucianoHanna
Copy link
Contributor Author

@k00ni I made these changes but there's one test failing, maybe should open another PR to remove "CI / Tests alternative autoloader (PHP 5.6)"

@k00ni
Copy link
Collaborator

k00ni commented Apr 23, 2021

#409 already targets the outdated alternative autoloader tests. Can you merge it and try again? In the end I will squash your PR, so it shouldn't matter

@LucianoHanna LucianoHanna requested a review from k00ni April 23, 2021 14:41
Copy link
Collaborator

@k00ni k00ni left a comment

Choose a reason for hiding this comment

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

Good to go 👍🏿

Will merge it soon.

@k00ni
Copy link
Collaborator

k00ni commented Apr 26, 2021

We had to fix some problems in our testing environment (Github workflow + Scrutinizer). All fixes are now in master. Can you please merge master to see if there are any problems left? Thank you for your patience.

@LucianoHanna
Copy link
Contributor Author

I merged master and everything looks ok but there's nothing left to commit...

@k00ni
Copy link
Collaborator

k00ni commented Apr 26, 2021

There should be a merge commit to push. Anyway.

@k00ni k00ni merged commit 57667ee into smalot:master Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants