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

Modernize code #218

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Modernize code #218

merged 3 commits into from
Oct 4, 2024

Conversation

eumiro
Copy link
Contributor

@eumiro eumiro commented Jul 15, 2023

  1. Use f-strings
  2. Use integers where appropriate
  3. Refactor generate_random

@eumiro eumiro changed the title Refactor generate_random Modernize code Jul 15, 2023
@rayrrr
Copy link
Member

rayrrr commented Nov 22, 2023

@eumiro Thank you for this PR! We're currently focusing on getting type annotations PR #203 merged first (FIFO).

Overall, I like what you've done here. However, in the GeoJSON spec in section 3.1.1, it states:

A position is an array of numbers. There MUST be two or more elements. The first two elements are longitude and latitude, or easting and northing, precisely in that order and using decimal numbers.

Emphasis added. The reason you see floats everywhere in the codebase, is because we would like this package to perfectly align with the spec. So, can you please change them back to decimal? From there, a rebase and merge should be all that's needed once we merge in the aforementioned other PR.

@rayrrr rayrrr self-assigned this Oct 4, 2024
@rayrrr rayrrr requested review from NoharaMasato and rayrrr and removed request for NoharaMasato October 4, 2024 13:43
Copy link
Member

@rayrrr rayrrr left a comment

Choose a reason for hiding this comment

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

Upon further review, #203 is not ready for merge, so we can merge this first. I'm merging this and will do a quick follow up PR to change the numbers back to float to match the spec.

@rayrrr rayrrr merged commit fe284f1 into jazzband:main Oct 4, 2024
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