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

Syntax, structure, indent and unneeded logic fixes #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jezek
Copy link
Contributor

@jezek jezek commented Sep 20, 2021

  • Fix unset usage.
  • Fix some missing -e in echo where needed.
  • Add some echos, before running some commands.
  • Minor alignment fixes.
  • Don't use 'newgrp' upon lxd install, seems to have no effect.
  • Unify command output redirection formatting.
  • Make some multi-line commands to be in one line.

Copy link
Member

@mardy mardy left a comment

Choose a reason for hiding this comment

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

Thanks, looks good overall, just a couple of nitpicks here and there :-)

crossbuilder Outdated Show resolved Hide resolved
crossbuilder Outdated Show resolved Hide resolved
@@ -276,22 +277,20 @@ config_container_dir_mount () {
}

start_container () {
STATUS=$(lxc query "/1.0/containers/${LXD_CONTAINER}/state" | \
jq --raw-output '.status')
STATUS=$(lxc query "/1.0/containers/${LXD_CONTAINER}/state" | jq --raw-output '.status')
Copy link
Member

Choose a reason for hiding this comment

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

I would reather keep this as it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May I ask why? I just tried to be consistent. There is nowhere in code splited line after pipe. And there are longer lines that are not splited just some lines above. So why split just this one? Also the splited original is inconsistently indented.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion on where the line should be split, but I have a preference for avoiding long lines.

If you want to make it more consistent and fix the indentation, you are very welcome to :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion the one-liner is more consistent and fits better with surrounding code. I presented my arguments, heard yours and come to conclusion, that I won't revert or change this. If you want it splited, do it yourself or don't merge this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've calmed down a little. I'm sorry, yesterday I was a little cranky.

Please, don't get me wrong, I don't want to be disrespectful and I want to resolve our small dispute about long line breaks. I still think, that the lines don't need breaks and your argument about 79 characters per line is invalid, because you don't follow the rule yourself. But I'm willing to compromise.

How about this. This (L:280)and the line in next conversation (L:293) will not be broken into 2 lines and the line in last conversation (L:664) will be split into 2 lines. Is this OK for you?

If not, then I propose to let the some other member/contributor to make the decision. We can make a pool and will follow the decision of the majority.

So @mardy , which one will you choose? Or do you have another proposal?

Copy link
Member

Choose a reason for hiding this comment

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

Let me be clear: I generally don't like formatting changes, they just add noise to the git history and make reverting commits harder.

If you proposed a commit which introduced a long line I most likely would not complain, because I don't think it's worth bothering contributors about it (given that we have plenty of them already). As you pointed out, I myself have added long lines, without being aware. I complained this time because you changed the formatting of an existing line (and this goes back to the first point: avoid formatting changes).

To sum it up, my philosophy is the following: I don't like formatting changes. They are fine, however, if they go in the direction of a style I'm more comfortable with.

So, while I appreciate that you proposed a compromise (joining these lines, and breaking some longer one), it is in fact not what I like, because it generates noise and those joined lines might be broken up again in the future...

I can also offer a compromise: do not touch these two lines, and feel free to add long lines (well, with a certain common sense :-) ) in your new code. :-)

Copy link
Contributor Author

@jezek jezek Sep 26, 2021

Choose a reason for hiding this comment

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

You clearly don't know what a compromise is. What you propose is not a compromise, so I can't accept it.

This commit doesn't exist, because I woke up & decided to make formatting changes. I was working on making the script work on non-debian based distros, literary manjaro (but you already know what I'm writing about). And when I work on something and I see some trash mess on the workspace, then I dispose of it, because it is distracting. A clean, uniform working environment is more efficient & less distractful for everyone. So in the process of reading & understanding and editing the code, I've got rid of those distractions. Then you pointed out that these small fixes should go to separate PRs, so here we are.

About these 3 lines. Whilst working on the manjaro fixes, I've stumbled upon the errors due to lxc info changes like @peat-psuwit did and made similar fixes as him. Then his changes got accepted, so I rebased and when resolving the conflict I joined the lines together. I folloved the logic, that the statements were one-liners before, so they should be a one-liners after. I accept, that the if statement is kind of long, but there is a longer if statement in the document and none of the 87 if statements are not multi-lined so why exactly this one? (see for yourself with grep -P '^\s*if\s' crossbuilder | awk '{ print length, $0 }' | sort -n -s | cut -d" " -f2- in the repo dir)

This is a community project, not yours. Requests should not be merged by what you are comfortable with or what you like, but what is logical and consistent. If you don't do formatting fixes because you don't like it, the code will go ugly and distractful in time and other people will be distracted by them and they will start to do formatting fixes and distract you even more.

Copy link
Member

Choose a reason for hiding this comment

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

I think you should try to be more respectful of other people's work. I'm not sure if by "trash" you mean the splitted lines that Ratchanan added, and which I happily accepted. I find broken indentation distracting at times, so I'm grateful that you want to fix that, but I find long lines more distracting than short lines, even if not properly indented.

As you wrote, this is a community project, and from my side I try to be welcoming of all contributions, accepting most of them as-is and not nitpicking on the format (up to a certain point, at least). The reason why we are hitting the wall with this PR is because it is changing the format itself: for such PRs, of course, I want us to try following my preferred style (which I assume is also Ratchanan's, since he deliberately avoided long lines in his commit), otherwise someone else (or even I myself) might come up some day with other cosmetic changes which partially revert the ones you are proposing here. That would just add noise in the git history, if we redo the same line over and over according to the preference of whoever happens to be "disturbed" by the current coding style.

Copy link
Contributor Author

@jezek jezek Sep 26, 2021

Choose a reason for hiding this comment

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

I'm sorry, trash was a unfortunate word. I meant it figuratively. I meant no disrespect toward others work. For this I humbly apologize. The better word would be a "mess" in meaning of "no order".

To avoid other cosmetic changes, you have to make clear rules. If there are no rules, uniformity should be pursued (which I do now). So, if you (and others) don't like long lines, write rules to code of conduct, or make all long line split (no only a few).

I say it again. I'm not here for quarrel. I pursue uniformity. You could always ask me to split all the long lines and I will do it (I don't mind doing repetitive work, as long it makes code more uniform). But then we must agree on line length. Your proposal of 79 chars is too conservative. I'm proposing the maximum line length for this project 140 chars (120 is acceptable too).

Another way to go is to say, that the lines we discuss about are splited because of beter readability of piped commands. Then I will split all commands with pipes and don't have a problem.

Make/choose clear sane rule(s) and I will follow them and gladly rebuild the source code according to them.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with your proposal to decide on a line length, and my preference, as already stated, is 79. I also believe that we shouldn't be too strict about this -- exceptions can be made, if in some cases staying within 79 chars makes the code over-convoluted.

@peat-psuwit, what is your preference?

crossbuilder Show resolved Hide resolved
if lxc query "/1.0/containers/${LXD_CONTAINER}/state" | \
jq -e '.network.eth0.addresses | any( .family == "inet" )' \
> /dev/null 2>&1 ; then
if lxc query "/1.0/containers/${LXD_CONTAINER}/state" | jq -e '.network.eth0.addresses | any( .family == "inet" )' > /dev/null 2>&1 ; then
Copy link
Member

Choose a reason for hiding this comment

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

also this one, please let it be in multiple lines :-)

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 agree, this one is longer, but still not the longest. There are 6 lines longer than this and not splited. So why this and not the other? Also the multi-line version is inconsistently indented too, and there is output redirection alone on the third line, which can mislead for which command it is for.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not very consistent on this, admittedly; but if we are making cosmetic style changes, let's go towards shorter lines please. :-)

Extra points if you propose splitting the other long lines too :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I may ask again? How do you differentiate between long and short lines, where is the line, where the short line becomes to long?

For me speaking, no line is too long. I use editor, which wraps lines. When I split, it is to improve readability of complicated 'if' statements. Also more lines will be shorter if the indentation is not 4 spaces (on this my opinion is to use 'tab' and anyone can set editor to his liking on how much spaces will be used for tabs).

Copy link
Member

Choose a reason for hiding this comment

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

A line width of 79 chars max is the most safe, and people can rely on that to keep multiple editor windows opened side by side without having automatic line wrap. Yes, most editors have automatic line wrapping, but it's still easier to read code where lines are split manually.

Copy link
Contributor Author

@jezek jezek Sep 23, 2021

Choose a reason for hiding this comment

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

So you want from me to keep the lines most 79 characters long and you introduced more longer lines few commits ago (eg. this one)? This lacks consistency from your side.

Edit: Btw. 79 chars for line is too short. Even Linus agrees with this.

@jezek
Copy link
Contributor Author

jezek commented Jan 22, 2022

@mardy So what about this PR? In meantime I've got one another approval. It seems, that @violethaze74 is OK (or doesn't care) about the split line merging.

@mardy
Copy link
Member

mardy commented Jan 22, 2022

@mardy So what about this PR? In meantime I've got one another approval. It seems, that @violethaze74 is OK (or doesn't care) about the split line merging.

Since this has been waiting for so long and that other contributors haven't voiced their opinion, I guess I'll just pick the changes that I like :-)

@jezek
Copy link
Contributor Author

jezek commented Jan 23, 2022

...other contributors haven't voiced their opinion, I guess I'll just pick the changes that I like :-)

I'm asking all contributors for this project (@fkaleo , @peat-psuwit , @fredldotme , @UniversalSuperBox , @jld3103 , @lduboeuf ), please read through our conversation (and this one too) and express your opinions and/or vote.

What to vote for?
Basicaly it is about line wrapping. My opinion is that that the 3 lines shouldn't be split, because:

  • No other line in the whole script is split after a pipe (|).
  • There are more longer lines (some of them introduced @mardy by himself), that are not split, so why this.
  • More uniform/consistent code is less distractible when reading through it.
  • Avoid formatting changes result in more messy code, that is hard to read.
  • Most editors have a (intelligent) long line wrapping feature, but none (that I know) has a split line merging feature.
  • Hard split lines occupy more vertical space. Today's monitors are more wider than higher and vertical space is scarce. So if you have lines split manually, there is less code to read without scrolling.

What are your opinions about the split lines merging in this PR?
What are your personal preferences for splitting lines manually (what is the maximum line length you allow in your code, after which you hard-split lines)?

I'm asking @mardy to wait until the end of January. After this month passes and there is no another vote/opinion (or the vote is a draw), I myself will revert the incriminating split lines merging (but fix the indentation).

Thank you all for your time and understanding.

@lduboeuf
Copy link
Contributor

My opinion: I don't care about conditions writen in one-line. In some cases it is even more readable. Of course not 200 chars long but < 100 is acceptable

- Fix unset usage.
- Fix some missing -e in echo where needed.
- Add some echos, before running some commands.
- Minor alignment fixes.
- Don't use 'newgrp' upon lxd install, seems to have no effect.
- Unify command output redirection formatting.
- Make some multi-line commands to be in one line.
@jezek
Copy link
Contributor Author

jezek commented Feb 6, 2022

@mardy From my perspective I've got an approval from a contributor to merge this, as it is. 😉 What's your understanding of the comment above?

@mardy
Copy link
Member

mardy commented Feb 6, 2022

@mardy From my perspective I've got an approval from a contributor to merge this, as it is. wink What's your understanding of the comment above?

I've stated my opinion before, I think it's clear enough. If you want me to merge it, please do not change existing lines to make them longer; that said, I'm certainly not going to start a fight if someone else merges your contribution in its current form. :-)

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.

4 participants