-
Notifications
You must be signed in to change notification settings - Fork 7
clean code and safely inspect fix. #5
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
Conversation
ctubbsii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these changes generated by a tool? There's a mix of stuff that's nice, but some other stuff that probably shouldn't happen. For one, there's a lot of unnecessary whitespace changes. We haven't applied our project formatter to our examples (yet), so that's probably fine for now, but it's a bit disruptive. We also tend to not suppress all warnings (we prefer to fix them), and also would prefer not to leave in commented-out code.
There's some good stuff in here, though, too... like limiting the visibility for fields, methods, and classes, when they don't need to be as visible.
I'm just not sure it's a great idea to merge in the current state.
|
Yes of course. It may be best to use the same format. also my goal is to be a developer. There have been changes made according to sonarqube results. We can make the changes by browsing together. be part of my target community. |
| } | ||
|
|
||
| config.getAppConfiguration().clear(); | ||
| // --Commented out by Inspection START (01.03.2019 15:22): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intellij IDE added auto comment line. I don’t looking check of control repeat. I’ll delete.
|
|
||
| public static final Column COUNT_SEEN_COL = new Column("count", "seen"); | ||
| public static final Column COUNT_WAIT_COL = new Column("count", "wait"); | ||
| public static final Column COUNT_SEEN_COL = new Column("count", "seen"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add the spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my formatter different yours. this line code false at Constants.class. i’ll delete. sorry.
|
@DogukanKundum Did you close this because you are no longer interested in contributing? Or because you do not have time to address the questions raised in the review? |
No. I want to contributor. But, first step i have your codeFormatter.xml. you send me codeFormatter.xml. after, I'll edit once this code changed. No problem bro. |
No description provided.