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

Resolve address number output issue and refactor road_formats for clarity #2138

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

semi-yu
Copy link
Contributor

@semi-yu semi-yu commented Nov 14, 2024

What does this change

This change addresses the issue in the previous implementation (#2133), where # was mistakenly printed instead of the actual address numbers. Additionally, the road_formats structure has been simplified by incorporating {{building_number}} directly for more concise formatting.

What was wrong

The original code had a formatting error where # was printed instead of the intended numeric address numbers. This caused unexpected outputs in the feature responsible for generating ROK (🇰🇷) road addresses (도로명 주소), leading to inaccuracies.

How this fixes it

The building_number method now correctly handles the generation of building numbers. It produces a standard building number or, with a certain probability, generates variations such as underground entrances using building_number_underground() or includes sub-building numbers via building_number_segregated(). This probability was determined arbitrarily, as there are no official government statistics available for this data.

Checklist

  • I have read the documentation about CONTRIBUTING
  • I have read the documentation about Coding style
  • I have run make lint

@stefan6419846
Copy link
Contributor

As this is a bug of the previous variant, shouldn't there be a test which checks for the previously broken behavior as well?

@semi-yu
Copy link
Contributor Author

semi-yu commented Nov 15, 2024

Hello,
You mentioned that it was a bug. However, there are a few points to consider:

  1. It's not so much a bug as it was my oversight. I forgot to add the bothify() function.
  2. This implementation fixes the part that was done clumsily. Instead of relying on adding strings to formats, I changed it so that the function generates the string.
  3. For string returns, I ensured that only str types are returned and modified the tests to check only for that.

Please review these changes. Since I'm using ChatGPT for communication, there may be some nuances lost in translation. Thank you.

@fcurella
Copy link
Collaborator

If addresses are supposed to never contain #, you could write a test like assert "#" not in building_number

@semi-yu
Copy link
Contributor Author

semi-yu commented Nov 21, 2024

Good morning from Korea.

Thank you for clarifying the task.
I have added test cases for the newly implemented building number functions.

  • Any string generated by building_number_*() must not include any '#'
  • Any string generated by building_number_underground() must begin with '지하' (which means "underground").
  • Any string generated by building_number_segregated() must include a dash ('-').

Please let me know if anything else is needed!

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.

3 participants