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

Changed POI id to String and removed hashCode. #555

Merged

Conversation

HilaryN
Copy link
Contributor

@HilaryN HilaryN commented Apr 27, 2024

To fix #554
This works, but I'm not sure it's a robust solution as I removed the hashCode override.
@mvl22 is the number within the POI ID unique? Or perhaps unique within category?

@HilaryN
Copy link
Contributor Author

HilaryN commented Apr 27, 2024

I've added the hashCode on the String, but not sure if this will be unique?

@mvl22
Copy link
Member

mvl22 commented Apr 28, 2024

No, the ID string as a whole is unique. I would simply use that as a unique value if you are having to index the points for some reason.

@mvl22
Copy link
Member

mvl22 commented May 6, 2024

@oliverlockwood @jezhiggins Could one of you review this so we can push this to beta or straight to live ideally? This continues to generate feedback and it’s clearly a priority for users.

Thanks Hilary for getting this checked out!

@HilaryN HilaryN marked this pull request as ready for review May 6, 2024 21:23
@oliverlockwood
Copy link
Member

This looks good in principle.

Before merging, I think we should:

  • on a local copy of the master branch, try temporarily removing the @Ignore annotation at the top of the RetrofitApiClientIntegrationTest and running the two POI-related UTs, namely hitGetPOIsByBboxApi() and hitGetPOIsByRadiusApi(). Based on my understanding of the issue, this should fail.
  • do the same test against the branch for this PR. This time these two tests should pass.
  • updating pois.json such that at least some of the POIs have alphabetical characters in their ID, not just numerical categories (I have just pushed a commit to your PR that does this)
  • this should make the RetrofitApiClientTest fail on master but pass on your branch.

Copy link
Member

@oliverlockwood oliverlockwood left a comment

Choose a reason for hiding this comment

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

I've now done the testing actions myself and am satisfied.

@oliverlockwood oliverlockwood merged commit 142a7b5 into cyclestreets:master May 7, 2024
1 check passed
@HilaryN
Copy link
Contributor Author

HilaryN commented May 7, 2024

Thank you!

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.

POI ID changed to string
3 participants