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

Update Maker.php : get country name by iso #16

Closed
wants to merge 1 commit into from

Conversation

visualight
Copy link

Retrieves the name of a country in different languages according to its ISO code.

Retrieves the name of a country in different languages according to its ISO code.
@petercoles
Copy link
Owner

happy with the new functionality, but can you add the tests for it?

@visualight
Copy link
Author

I'm very annoyed ... I've never used tests in my life as a developer for 25 years. I don't even know how it works 😂

$country = CountriesFacade::getCountryName($user->country, 'fr'); // Get user country in french (ex.: Belgique)
dd($country);

@petercoles
Copy link
Owner

25 years. You've a newbie :-) I've been developing since the 1970s and managing complex projects since the early 80s. If I could only pick one of the many changes I've seen in that time, automated testing would be the most game-changing advance to the development process.

The tests aren't just to ensure that your code does everything it's supposed (e.g. handle the case when an invalid iso code parameter is passed), it's actually more important for ensuring that future changes made by people who aren't familiar with your addition don't accidentally break it, and potentially break the thousands of third-party applications using the package.

BUT it was a good idea, so I've re-implemented it slightly, together with the tests needed and additions to the documentation, and credited you in the commit message.

@petercoles petercoles closed this Sep 8, 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.

None yet

2 participants