Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable autoformatting #392
Enable autoformatting #392
Changes from 12 commits
344a6dc
766b738
2becaa3
d856f83
ea6e28c
a7e0117
57cff1b
1a7dea0
9f6c393
7530956
3fb49cb
42a5d3d
e14a312
43a9950
e6d4db9
42365fb
a828b88
418e274
47d2a93
6ddecf8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Switched to a simple alphabetical order IIUC? Very welcome. Humans should not be reading
import
statements to begin with; normalizing the order minimizes the chance of stupid merge conflicts.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.
humans do code review.. whilst you may not read them and you may feel they are unwanted others like me find them very useful.
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.
@jglick
It is my understanding that what code style guidelines there are for Jenkins say "Simple alphabetical non-static, then simple alphabetical static". Is that incorrect?
@jtnord
What you prefer the ordering be? I don't care what it is as long as I never have to tell someone in a code review to change it.
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.
So long as it is simple I would not really care. (Whether
static
import
s should be allowed at all, and under what conditions, is a contentious topic.)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.
@jglick
The topic of
static
import
s will not be solved by a formatter.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.
I have personal preferences but I do not care too much about the order apart from always non static before static imports though, and static imports should be reserved for test code apart from those very rare special occasions.
But given as you asked, my preference is someone along the following lines.
java
,javax
)com
.edu
,org
)hudson
,jenkins
,org.jenkinsci
,io.jenkins
)org.jenkinsci.plugins.github_branch_source
)That said my IDE will automatically an import put it where it is configured to by its ordering (eclipse) and having per project settings is a PITA (I do not recall the last time I typed an import statement). I try to reduce diff bloat it after the fact by manually moving it if I recall :)
you mean the ordering or that they should or should not be allowed?
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.
Whether or not they should be allowed.
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.
FWIW my personal preference is strictly alphabetical, no line breaks, no attempt to group in any way other than
| sort
.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.
I would say this is less readable than before.
is more readable to me .. but 🤷 you choose somethign and it gives you something.
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.
I can see the argument that
throws
should go on the same line. But whether this less readable is debatable - do you really want to debate it?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.
you can choose to disregard my opinion here and I am ok with that.
hence
auto formatters will always produce something that is worse. this is to me one (all be it a very minor) case of that.
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.
should be aligned to
writeLog(
notchecker
.basically are you using a fixed offset for wrapped indentation? (ie 8 which is the lengh of
checker.
- in which case don't use an offset that matches to the thing you are offsetting.otherwise you can see something like
and that is worse than evil.
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.
@jtnord
Same debatable point as above. The indent you are saying is missing was (as far as I can tell) intentionally removed as redundant.
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.
@jtnord
Regarding the align to column, you can change this the formatter as noted here to be
82
and it will align to column, but it produces results that I think you'll like even less than the current results.50
is also an option, but it ends up even worse.But maybe I'm misunderstanding what you mean.
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.
so given the settings that are being used are offset by a specific amount (and not column) if it hard to see when something is wrapped in something like
Because both
jennygoestomarket
andmarkstaysathome
areoffset
by the same amount, and when scanning the code it looks like they can be the arguments to the same function (when they are not).Whitespace is helpful to show what things apply to (which is why we indent blocks
{
) to me the same applies here.Maybe I misunderstand the settings you are applying? (but I have had many issues with eclipse when using this setting in the past)
disclaimer #1 - I much prefer projects that are set to indent on column.
disclaimer #2 I am mildly dyslexic and so this is important to me. My brain scans and reads text in a specific and different to the vast majority of people.
now - I am not a major contributor to this project and so I am not trying to enforce my standards onto the people who do work with it much more than I do (I have only had a few minor contributions) and as such I will adapt when I need to work in this project. However if this is the first step into trying to get something more standard into Jenkins ecosystem and rolling out more widely then being inclusive of all people (which includes the neuro diverse) should be taken into account, as descisions made here can make it more or less easy for people to contribute.
disclaimer #3 on my projects when I remember to switch to my coding style I
indent
with tabs so that people can customize their indent view to support their needs (2 for some people 8 for others, and some have 4 or even 3) butoffset
with spaces. this allows the code to look the same regardless of tab space setting and allow people to have a visual offset that conforms to the way they work best.and finally disclaimer #4 all attempts at auto formatting will break somewhere. This is why I think auto formatters are evil. let the bot do a formatting review, let a human decide to reject it's findings, or to easily make it better. See the end part on disclaimer 2
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.
@jtnord
The problem I've seen with with indenting to column is you end up with deeply indented code. I assume your suggestion for addressing that is to refactor the code into smaller methods, avoiding deeply nested structures. Am I correct?
Neuro-atypicalness and accessibility is certainly a valid concern. I read slowly and also depend on formatting to allow me to take in code at a reasonable speed. I definitely want to be sensitive to a broad range of contributors needs. At the same time, consider that someone else might find "align to column" harder to read due to its deeper indenting causing lines to wrap more often. Which one wins? In the case of this change, I am looking to apply a standard formatting that most closely matches what is already being used. There are parts of this code base that seem to be using "align to column", but the majority of it does not.
I believe this mix of tabs and spaces is referred to as "smart-tabs". It works best in "align to column" if I understand it correctly. I'm not opposed to this, but as with point two it seems a larger change. I've done like I did with no-join and created a branch using this and "align to column" settings: bitwiseman:task:task/formatting...bitwiseman:task/formatting-columns. Perhaps there's other settings that could be applied to do this better. Perhaps you as the one bringing this up could try various setting to find one that is best in your opinion?
I would instead say that all attempts at formatting will be imperfect, both humans and automated systems. Auto-formatters are generally imperfect in rigid and predictable ways, whereas human formatters are imperfect in a wide range of random ways. I categorically disagree that humans should be allowed to opt-out of auto-formatting - given the option humans will choose to do things their own way and then we end up talking about "better" or "worse" formatting instead of focusing on making the code better.
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 preference would be to use the autoformatter for things it can do reliably—sort imports, remove trailing whitespace, enforce spaces vs. tabs for block indentation, normalize use of spaces around / inside tokens like
(
)
=
—but for more subjective things like placement of line breaks leave the choice to humans.