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 ISBN validator #316

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Refactor ISBN validator #316

merged 1 commit into from
Jul 3, 2024

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Jul 3, 2024

Q A
Documentation yes
BC Break yes
QA yes

Description

  • Removes option setters and getters
  • Drops the separator option in favour of stripping them from the input
  • Inlines checksum calculation, removing unnecessary classes
  • Adds types
  • Accept only an options array to the constructor
  • Simplify tests with data providers

- Removes option setters and getters
- Drops the `separator` option in favour of stripping them from the input
- Inlines checksum calculation, removing unnecessary classes
- Adds types
- Accept only an options array to the constructor
- Simplify tests with data providers

Signed-off-by: George Steel <[email protected]>
@gsteel gsteel force-pushed the v3/refactor-isbn branch from 441b323 to 7aff015 Compare July 3, 2024 11:19
gsteel added a commit to gsteel/laminas-validator that referenced this pull request Jul 3, 2024
More info in laminas#316

Signed-off-by: George Steel <[email protected]>
@gsteel gsteel mentioned this pull request Jul 3, 2024
Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

Nice improvement! 👍🏻

@gsteel gsteel self-assigned this Jul 3, 2024
@gsteel gsteel merged commit 5575df2 into laminas:3.0.x Jul 3, 2024
14 of 15 checks passed
@gsteel gsteel deleted the v3/refactor-isbn branch July 3, 2024 11:26
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