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

Prevent breaks on future update on version number #595

Closed

Conversation

afischerdev
Copy link
Collaborator

This update aims to avoid breaks in static functions when merging PR #457.

@afischerdev afischerdev temporarily deployed to BRouter July 18, 2023 12:15 — with GitHub Actions Inactive
@rkflx
Copy link
Collaborator

rkflx commented Jul 18, 2023

For anyone wondering what this is about (given the sparse description and ambiguous title) and wanting to save some time scratching their head:

It's basically a compile fix for rebasing #457 on master. 3c5ac66 recently added static functions with read access to the version variable. However, #457 will change version to be a function accessing the non-static method getClass(), so the newly added functions won't compile anymore unless they are made non-static too, like some other functions in this class.

That being said, I'd solve the issue differently. Using the following patch for #457 should work too, with less overhead and being more broadly applicable (i.e. also usable in static functions):

public final class OsmTrack {
-  final private String version = getClass().getPackage().getImplementationVersion();
+  final private static String version = OsmTrack.class.getPackage().getImplementationVersion();

@afischerdev
Copy link
Collaborator Author

For anyone wondering what this is about (given the sparse description and ambiguous title) and wanting to save some time scratching their head:

Please look after your words.

There are always more options.
But I agree your way is more elegant and I shouldn't have done anything.
So I can close this and let others play the ball.

@rkflx
Copy link
Collaborator

rkflx commented Jul 19, 2023

I'm sorry if you felt the wording being too blunt. My intention was not to pick on you personally, but to point out why someone other than the author might have trouble understanding some of the PR. If your are not interested in this kind of feedback, that's okay. I guess I should have worded it as yet another "What do you mean by that?"-inquiry too common in this project.

At least I initially took a while to understand this is not about a breakdown once a version number will be increased, and why removing static would solve anything. If other readers of the PR are more clever, good for them.

Anyway, good luck finding other reviewers for your PRs, I won't bother anymore for the most part.

@afischerdev afischerdev deleted the prepare-version-change branch July 20, 2023 10:43
@afischerdev
Copy link
Collaborator Author

@rkflx
I'm sorry, I missed your statement while closing.
The feedback is not the problem, support, quality assurance, improvements, ... are welcome and needed.
The thing is the style of communication, there are different ways of presentation.

@rkflx
Copy link
Collaborator

rkflx commented Jul 21, 2023

My focus will be elsewhere going forward, sorry for the noise.

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