Skip to content

Conversation

@julio-cavallari
Copy link
Contributor

@julio-cavallari julio-cavallari commented Jan 26, 2025

What:

  • Bug Fix
  • New Feature

Description:

Some country codes and currency codes are detected as misspelling, so I created these stubs with all country codes in 2 and 3 digit formats, and all currency codes, following ISO standards

Stubs added:

Related:

#123

@julio-cavallari julio-cavallari changed the title Add currency code stub Add stubs for ISO 4217 and ISO 3166 alpha-2 and alpha-3 Jan 27, 2025
@c0nst4ntin c0nst4ntin linked an issue Jan 27, 2025 that may be closed by this pull request
@c0nst4ntin c0nst4ntin added the enhancement New feature or request label Jan 27, 2025
Copy link
Collaborator

@c0nst4ntin c0nst4ntin left a comment

Choose a reason for hiding this comment

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

@julio-cavallari The code looks good (as per usual with your pull requests) 😉 Good work!

Just one idea, though I was having. We are starting to extend the base/chosen preset with a lot of things. Maybe people don't necessarily need/want this. We could think about making these additional things configurable.

Either through a new config value, or by turning the preset option into an array.

@julio-cavallari
Copy link
Contributor Author

@julio-cavallari The code looks good (as per usual with your pull requests) 😉 Good work!

Just one idea, though I was having. We are starting to extend the base/chosen preset with a lot of things. Maybe people don't necessarily need/want this. We could think about making these additional things configurable.

Either through a new config value, or by turning the preset option into an array.

@c0nst4ntin I agree, how do you plan on doing this? I think if we don't think it through, the config could start to get very complex.

@c0nst4ntin
Copy link
Collaborator

c0nst4ntin commented Jan 27, 2025

@julio-cavallari We could change the config to accept:

{
    "presets": ["base", "laravel", "iso4217", "iso3166"],
    // ...
}

Or we could always include the base preset and list the other presets like laravel etc. in the config:

{
    "presets": ["laravel", "iso4217", "iso3166"],
    // ...
}

What do you think? Maybe we should also get @nunomaduro Feedback on this?

@julio-cavallari
Copy link
Contributor Author

@c0nst4ntin I think it's a good approach

But do you think this should be done in this PR? I believe it would be best to separate it into an exclusive PR, so as not to mix two features.

About the base stub, I believe that this one specifically needs to be always active, what do you think? And in it we only put words that we are sure will need to be ignored as typos in any project.

@c0nst4ntin
Copy link
Collaborator

@julio-cavallari I think we can first get this added into the project and then change the presets field to an array in the config in another pull request if we see the need!

Will hand this to @nunomaduro

@c0nst4ntin c0nst4ntin merged commit 2d66c8a into peckphp:main Mar 9, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Currency codes are flagged as typos

2 participants