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

Refactoring with Lombok #57

Merged
merged 10 commits into from
Nov 6, 2020
Merged

Refactoring with Lombok #57

merged 10 commits into from
Nov 6, 2020

Conversation

bonaparten
Copy link
Contributor

@bonaparten bonaparten commented Oct 8, 2020

For further discussions: #50

@bonaparten bonaparten linked an issue Oct 8, 2020 that may be closed by this pull request
@FabiKo117 FabiKo117 self-requested a review October 8, 2020 08:25
Copy link
Contributor

@FabiKo117 FabiKo117 left a comment

Choose a reason for hiding this comment

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

see inline comments

@FabiKo117 FabiKo117 added this to the 1.2 milestone Oct 27, 2020
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

I found some formatting/checkstyle issues.

regarding the main "content" of the PR, I'm missing a bit the rationale behind these changes. I see the amount of "boring" code like getter/setters is reduced. But it comes (at least in my IDE IntelliJ) at the cost that helpful stuff like code autocompletion doesn't work anymore. Or is there a trick to get this integrated into an IDE which I don't see right now, e.g., does one need to install a plugin or something?

//edit: I tried the instructions from https://www.baeldung.com/lombok-ide, but didn't have much luck. 😞
//edit2: ok, after a second try it worked (I must have misclicked something the first time). anyway, this really needs to be documented somewhere (readme, contrib. guide, …) IMO.

pom.xml Outdated Show resolved Hide resolved
&& "remainder".equalsIgnoreCase(jsonNode.get("groupByObject").get(1).asText()))
.findFirst().get().get("result").get(0).get("value").asInt(), 0);
assertEquals(43,
StreamSupport
Copy link
Member

Choose a reason for hiding this comment

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

why was the code formatting changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no answer for that, it is automatically formatted. I'm not using specific format settings which regard this issue.

Copy link
Member

@tyrasd tyrasd Oct 29, 2020

Choose a reason for hiding this comment

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

ok, I see. If you ask me, this is not a good situation to use automatic code formatting, for multiple reasons:

  • First, the code was already correctly formatted, so why change it?
  • Since we're not all using the same IDEs, if another person uses a different auto code formatter, the same line would potentially be changed again. This could result in an "edit war" of the auto formatters.
  • Such changes make it hard to read and review a pull request, because more lines are touched and it is sometimes hard to see whether something really changed or if it was "just" the reordering of a few newlines and whitespaces.
  • Most importantly, any such line which is touched without a reason can lead to unnecessary merge conflicts with other branches. This can cause lots of manual work down the line.

You might choose to use an auto-formatter during your own development however you like, of course. But if you do, please make sure not to commit unnecessary changes which were just introduced automatically. 🙏 git add -p is your friend.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also ran into that issue in the past (and sometimes sitll today), which can indeed be a bit annoying and missleading.

What I try to remember myself always now is just marking the specific code lines, or method, that I've added/edited and then apply the auto formatting on that, instead of the whole class.
Sometimes it really changes something on a completely other part of the code, which also fits to the coding guidelines, but is just another way of ordering the lines..

@bonaparten
Copy link
Contributor Author

I found some formatting/checkstyle issues.

regarding the main "content" of the PR, I'm missing a bit the rationale behind these changes. I see the amount of "boring" code like getter/setters is reduced. But it comes (at least in my IDE IntelliJ) at the cost that helpful stuff like code autocompletion doesn't work anymore. Or is there a trick to get this integrated into an IDE which I don't see right now, e.g., does one need to install a plugin or something?

//edit: I tried the instructions from https://www.baeldung.com/lombok-ide, but didn't have much luck. disappointed
//edit2: ok, after a second try it worked (I must have misclicked something the first time). anyway, this really needs to be documented somewhere (readme, contrib. guide, …) IMO.

I can write something on confluence about it or elsewhere. Where is it better? @FabiKo117 @tyrasd

@tyrasd
Copy link
Member

tyrasd commented Oct 29, 2020

IMHO, for now https://github.com/GIScience/ohsome-api#getting-started seems like the best place to start. Or alternatively start a github wiki page.

Rosario Trischitta and others added 10 commits November 6, 2020 09:32
making construct private
deleting construct annotation
fitting methods
fitting junit tests
not prioritizing domain package import
formatting pom.xml
to use 2 spaces instead of one tab
to be in alphabetical order
Copy link
Contributor

@FabiKo117 FabiKo117 left a comment

Choose a reason for hiding this comment

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

looks good now 👍

@FabiKo117 FabiKo117 merged commit 7c04e84 into master Nov 6, 2020
@FabiKo117 FabiKo117 deleted the lombok branch November 6, 2020 12:29
@tyrasd
Copy link
Member

tyrasd commented Nov 17, 2020

360508a changed the JSON output of all groupBy requests from groupByObject to groupByObjectId. this must have slipped through in the code review. 🙈

@tyrasd tyrasd mentioned this pull request Nov 17, 2020
@tyrasd tyrasd added the code quality Topics around code quality, e.g. refactoring, better naming of methods/classes label Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Topics around code quality, e.g. refactoring, better naming of methods/classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introducing Lombok to code to reduce verbosity
3 participants