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 new parameter collector for BRouter app #634

Merged
merged 9 commits into from
Nov 17, 2023

Conversation

afischerdev
Copy link
Collaborator

Use the new parameter collector also in app context and remove old param logic and routines.

@afischerdev afischerdev temporarily deployed to BRouter October 5, 2023 11:01 — with GitHub Actions Inactive
@quaelnix
Copy link
Collaborator

Lots of duplicate code gone for good! 👍

... however, I'm a little afraid that this might break something, though I haven't found anything yet.

@afischerdev
Copy link
Collaborator Author

@quaelnix

however, I'm a little afraid that this might break something, though I haven't found anything yet.

Indeed it has the potential to run into problems.

My ideas on this situation:

  • finish the current param collector for BRouter web service
  • add a new output logic (move from OsmTrack to extra classes) - this also carries a risk in it

In my opinion, the server is not critical. It could change the library very quickly if something doesn't work properly. And these kind of changes can be easily tested by comparing the results - there should be no change. But for the app an update on trouble will take some days. And this is not easy on testing.

It could be a good way to use a beta app via the Play Store to have more test with users. But we can't publish an update without any user relevant changes.
So I would like to add changes on voicehint #613 as well.

@afischerdev afischerdev temporarily deployed to BRouter October 18, 2023 10:52 — with GitHub Actions Inactive
@quaelnix
Copy link
Collaborator

add a new output logic (move from OsmTrack to extra classes) - this also carries a risk in it

If it improves the readability of the code, I'm in.

In my opinion, the server is not critical. It could change the library very quickly if something doesn't work properly.

I think we should try to break neither of the two. We can't know who is using the server in what way.

@afischerdev
Copy link
Collaborator Author

If it improves the readability of the code, I'm in.

It only clears the OsmTrack from formatting and move it to Gpx, Kml, Json, Cvs classes. But code is as it was before. Map matching will need 5x5 OsmTracks to compare each point with next.

I think we should try to break neither of the two.

Yes, of course, but how do you achieve this goal?

@afischerdev
Copy link
Collaborator Author

@quaelnix

I think we should try to break neither of the two.

Is there a new idea to protect the server lib or/and the app from breaking?

@zod
Copy link
Collaborator

zod commented Nov 7, 2023

Getting rid of duplicated code is always desirable because it's less code to maintain :) Thanks for your effort!

The usual approach to ensure refactoring doesn't break functionality is to write tests before refactoring which represent the desired behavior. Unfortunately most BRouter classes can't be tested in isolation because of close-coupling with other classes which makes writing unit tests really hard. Usually coverage information is also used to have a metric/information which parts of the code are covered by tests.

@afischerdev
Copy link
Collaborator Author

@zod
My extra tests are no unit tests.
I use the server and fire around 80 requests for old lib against new lib. And compare the results.
For the parameter and the new output logic the tests are easy. The result must be the same. That covers the most features we have.
Some day I like to do that with an additional Android app as well.

@quaelnix
Copy link
Collaborator

quaelnix commented Nov 9, 2023

Is there a new idea to protect the server lib or/and the app from breaking?

No, and I also do not understand this part of the code well enough to be of any help in this matter. Sorry.

@afischerdev afischerdev merged commit c647305 into abrensch:master Nov 17, 2023
1 check passed
@afischerdev afischerdev added this to the Version 1.7.4 milestone Jan 17, 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.

3 participants