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

Move constants out of global namespace #38

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

MatheusRich
Copy link
Contributor

Defining constants in the global namespace is dangerous as it might conflict with other libraries/apps.

Note: This is a breaking change.

require_relative 'itext'

class FillablePDF
class Field
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be a module instead, since we're not creating any instances

@vkononov
Copy link
Owner

Given that this is a break change, did you actually encounter an issue where you had a conflict with another gem or your own code?

@MatheusRich
Copy link
Contributor Author

Yes, and I think this will keep people from using this gem.

@vkononov
Copy link
Owner

Thanks for the pull request. I will try to review and merge it right away and release a new version of the gem.

@vkononov
Copy link
Owner

The changes look great. The only thing that is missing is updates to README.md. Would you be able to make those changes?

@vkononov
Copy link
Owner

I was trying to resolve conflicts between this branch and the previous branch you had pushed, which I recently merged, and it looks like it broke something. :(

@MatheusRich
Copy link
Contributor Author

@vkononov I think the tests failing are because we've merged #37. I'll rebase.

Defining constants in the global namespace is dangerous as it might conflict with
other libraries/apps.

Note: This is a breaking change.
@MatheusRich
Copy link
Contributor Author

@vkononov I've updated the references to Field to FillablePDF::Field in the README. Is there anything else you'd like me to change?

@vkononov
Copy link
Owner

Looks great. Thank you, @MatheusRich, for making these changes. Will merge right away and release a new version of the gem sometime today.

@vkononov vkononov merged commit f6bd5e6 into vkononov:master Jul 12, 2023
8 checks passed
@MatheusRich MatheusRich deleted the move-constants branch July 12, 2023 14:31
@MatheusRich MatheusRich mentioned this pull request Jul 12, 2023
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