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.

Sign up for GitHub

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

Jump to bottom

feat: first implementation of OSM Hiking route API #22

Merged
merged 10 commits into from
Oct 12, 2024
Merged

feat: first implementation of OSM Hiking route API #22

merged 10 commits into from
Oct 12, 2024

Conversation

Wylwi
Copy link
Contributor

@Wylwi Wylwi commented Oct 8, 2024 •
edited
Loading

Summary

  • Feature was added to include a first standard HikeRouteRepository to fetch Hiking routes from Overpass API (Overpass API is the API to extract data from OpenStreetMap)

Details

  • Json Parsing is done using a Reader, since otherwise we can easily run into an OOM error when reading the whole body as a String (body can easily reach MB).

Remark: PR is 560 lines long, but a lot of lines are really test data, and I have put detailed comments.

@Wylwi Wylwi linked an issue Oct 8, 2024 that may be closed by this pull request
Integrate the Open Street Map API for the map #20
Closed
@MaelImhof
Copy link
Contributor

Looks good to me, it needed to be done indeed. However did you open the PR from the branch where you work on the hiking routes model?

@MaelImhof MaelImhof self-requested a review October 10, 2024 08:32
@Wylwi
Copy link
Contributor Author

Wylwi commented Oct 10, 2024

@MaelImhof This is a draft PR, not ready for review :)

Thus nothing is ready to merge nor review 😅

@HikeMate HikeMate deleted a comment from sonarqubecloud bot Oct 10, 2024
@HikeMate HikeMate deleted a comment from sonarqubecloud bot Oct 10, 2024
@Wylwi Wylwi removed the request for review from MaelImhof October 10, 2024 08:49
@MaelImhof
Copy link
Contributor

Right, sorry about that, I thought it was just a change of dependencies 😅

@Wylwi Wylwi self-assigned this Oct 10, 2024
@Wylwi Wylwi force-pushed the feat/20-integrate-the-open-street-map-api-for-the-map branch from 83a095d to 0eaf6ff Compare October 10, 2024 21:46
@Wylwi Wylwi marked this pull request as ready for review October 10, 2024 21:46
@Wylwi Wylwi changed the title fix: bump osmdroid version First implementation of OSM Hiking route API Oct 10, 2024
@HikeMate HikeMate deleted a comment from sonarqubecloud bot Oct 10, 2024
@mchac15 mchac15 self-requested a review October 11, 2024 05:50
@Wylwi Wylwi changed the title First implementation of OSM Hiking route API feat: first implementation of OSM Hiking route API Oct 11, 2024
@mchac15
Copy link
Contributor

mchac15 commented Oct 11, 2024

Overall the code is very well done. It is really organized, specially how you parse the routes with all the helper methods; this is a great way to organize the code. I just have a couple of comments which don't critique the quality nor the functionality of the code but more little details to be improved.

mchac15
mchac15 previously approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@mchac15 mchac15 left a comment

Choose a reason for hiding this comment

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

To conclude: The code seems well written, after reading it for a while I don't see any major problems.
After making the two little changes you can merge it.

app/src/main/java/ch/hikemate/app/model/route/HikeRoute.kt Show resolved Hide resolved
app/src/main/java/ch/hikemate/app/model/route/HikeRoutesRepositoryOverpass.kt Outdated Show resolved Hide resolved
Wylwi added 7 commits October 11, 2024 22:40
@Wylwi
fix: bump osmdroid version
ee63bab
@Wylwi
feat: implement basic datastructures for hike routes
cb7ee55
@Wylwi
feat: add support for hike route fetching with OSM
2154509 
Added a repository interface for providing routes, and an implementation of this interface based on the Overpass API, which is an API for reading and extracting data from OSM
@Wylwi
chore: move the ListOfHikeRoutesViewModel
fafcc55 
The ListOfHikeRoutesViewModel is more suitable to be in model.route than model.map, since it doesn't provide any data for the map itself, just data to show on top of it
@Wylwi
chore: add missing okhttp dependency
9dd3228
@Wylwi
fix: change incorrect latitude mapping
7e37c01 
In Overpass implementation of HikeRoutesRepository, a longitude was set as a latitude
@Wylwi
test: add initial tests to HikeRoutesRepositoryOverpass
a978364
@Wylwi Wylwi force-pushed the feat/20-integrate-the-open-street-map-api-for-the-map branch from 0eaf6ff to a978364 Compare October 11, 2024 20:40
@rBergCode rBergCode mentioned this pull request Oct 12, 2024
Closed
1 task
@HikeMate HikeMate deleted a comment from sonarqubecloud bot Oct 12, 2024
@Wylwi Wylwi requested a review from rBergCode October 12, 2024 17:28
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mchac15
mchac15 approved these changes Oct 12, 2024
View reviewed changes
Copy link
Contributor

@mchac15 mchac15 left a comment

Choose a reason for hiding this comment

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

LGTM

@Wylwi Wylwi merged commit b7ef5a5 into main Oct 12, 2024
2 checks passed
@Wylwi Wylwi deleted the feat/20-integrate-the-open-street-map-api-for-the-map branch October 12, 2024 18:55
@MaelImhof MaelImhof added sprint 1 Active during the first sprint sprint 2 Active during the second sprint labels Nov 15, 2024
@MaelImhof MaelImhof added this to the Milestone 1 milestone Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchac15 mchac15 mchac15 approved these changes

@rBergCode rBergCode Awaiting requested review from rBergCode

Assignees

@Wylwi Wylwi

Labels
sprint 1 Active during the first sprint sprint 2 Active during the second sprint
Projects
None yet
Milestone
Milestone 1
Development

Successfully merging this pull request may close these issues.

Integrate the Open Street Map API for the map
3 participants
@Wylwi @MaelImhof @mchac15