-
Notifications
You must be signed in to change notification settings - Fork 446
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
Remove trailing whitespaces. Add CI script to verify in on every commit #5225
Remove trailing whitespaces. Add CI script to verify in on every commit #5225
Conversation
@CharlieFenton, @davidpanderson, Please let me know what you think about it. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5225 +/- ##
=========================================
Coverage 10.87% 10.87%
Complexity 1064 1064
=========================================
Files 279 279
Lines 35977 35977
Branches 8255 8255
=========================================
Hits 3911 3911
Misses 31674 31674
Partials 392 392
|
Fine with me. Charlie, any objection? |
I was afraid this would be a significant problem for me, but I think I've figured out how to make it only a significant inconvenience. Because I have written so much code for BOINC over so many years, I often can't remember why I did something a particular way. I use blame / annotate quite a bit to help me find the commit message and the associated PR with one or more lines of code I wrote, as well as code that others wrote. This helps me avoid reintroducing old bugs. The ability of git blame / annotate to ignore whitespace by adding the -w argument is not as helpful to me as you think, because I never use git on the command line. I find git on the command line much less useful or easy to use as the GUI application SourceTre, which is a GUI wrapper for git. My problem is that, unfortunately, SourceTree does not provide a way to add the -w argument to git's blame command, despite many requests by users for that feature. SourceTree allows me to just click on a line of code and automatically shows me the commit message and date, making it easy for me to find the corresponding PR. This takes considerably more effort on the command line. SourceTree also allows me to directly view the commit in the repository history which makes it even easier to see the merge which lists the PR. I spent several hours last night working out a hybrid method to work around the lack 0f -w in SourceTreee. First, I get the basic blame info using git on the command line. I then will have to invoke blame for the same line of code in SourceTree. SourceTree shows me a series of commits, beginning with the commit that only removed blank space. With the date and SHA of the change I got from the command line (with -w), I can then look over the list from SourceTree to find the commit that the command line found (the one I actually want) and proceed from there to read the commit message or look for the merge commit, a little later in the SourceTree's history view, which then gives me the PR number. Hopefully, the comments in the PR will tell me what I need to know. Clearly, this hybrid method is less convenient than what i do now, but I can live with it. I have another reason I don't line this, which I will put in the next comment in this PR. |
I also don't like this PR is because I strongly disagree that blank space at the end of lines is a problem that needs fixing. In fact, I have often intentionally added a space at the end of a comment line, especially when a comment is several lines long. It makes it a bit easier to make changes to the comment which then make some lines too long or too short. But that is a minor convenience. But my main objection is that this is a solution in need of a problem. |
I have another objection. Modifying 743 files changes their modification dates, which I also often use to help quickly determine what feels have changed since a certain date. Yes, i know there are other ways to get that information, but modification dates ar quick and easy to check. In fact, I keep a separate local repository on my computer for every branch and tag to avoid changing the branch or tag using git checkout because that messes up the modification date. In general, changing 743 files for a non-issue like this is simply crazy, IMHO. Perhaps I am a dinosaur, continuing to use these old methods. But that's what happens when one has earned his living writing code for over 50 years as I have. |
Also, adding a CI script to flag trailing blank space as an error goes too far and will waste people's time. As far as I know, no compiler flags trailing spaces as a warning, and certainly not as an error. |
It's mostly about the clean code. I think that this PR will be helpful even for you, because when next time you will just do git blame without ignoring whitespaces, you will see that some particular line was changed in this PR with the clear description and not in a scope of other changes in a several other PRs. Basically, my intention here includes next options:
So I ask you to think about this PR one more time. Thank you in advance. |
There is no way I will examine 743 files by eye. I feel you have already wasted too much of my time on this unnecessary and ill-advised undertaking. I am willing to examine the script or other method you used to modify the 743 files, which is not explained anywhere in this PR. If that script is one of the 743 files, please point it out.
@AenBleidd I object very strongly to this. Waiting for CI checks to complete is already a part of the development process that makes it especially slow and tedious. It is quite disrespectful of people's time to require them to go through another round of changes because they had an extra space in their code that had no actual effect. If you feel it is important to remove such blank space (and I disagree), it should be done by periodically running a script to clean up the files. |
I kinda agree with Charlie. It ain't broken, and we have many more important things to worry about. |
Ditto. Like, concentrating on the logical integrity and functionality of the end product for a while. |
Charlie, I highly respect you and your work. Basically, I do programming more years than I'm alive, and that's smth crazy to me (in a good sense of meaning). However, citing 'Deux Ex: Mankind Divided' videogame: 'The world is different now. Old rules no longer apply.' If we want BOINC to survive and develop, we should think about other contributors who would like to help BOINC, and we should encourage doing that, not discourage.
That's not the git way, and I know no one except you who does this.
CI is made to simplify the work on reviewers, and usually CI includes some styling checks as well, because this is smth contributors would like to have, and not spend too much time every time to every new contributor. So, having this on CI is completely fine.
That's true, but nowadays people uses not compiler only but rather a set of tools, that do all necessary job. For example, several times you told that the PR should not include unneeded whitespaces removal. I can understand this. But you should also understand that a lot of tools doing this automatically, and you can't expect anyone will adjust their tools' setting to the one particular project.
This was a part of the script that is now a CI check. I intentionally removed that from the script because this is smth that should not be done on CI. If you want - I can put those changes back and send that script to you, but I don't feel necessary to keep it inside our repo (only in case if you want users to run it before committing to avoid CI errors).
Basically, if we don't do this now in one PR, then we should not complain when any PR will contain whitespaces removal. But if we merge this PR, we will have no issues with this anymore, because this is a precondition, and CI will show errors in the lines that should be fixed.
Having clean codebase and easier PR review process is important as well. If you're not working alone on the project, you should do smth to make collaboration easier. And this particular PR tries to fix exactly this problem. |
@AenBleidd I can live with this if you and David feel it is necessary. Regarding making CI fail if there is trailing whitespace in any file, it will no longer affect me because I have now changed my Xcode settings to automatically remove trailing whitespace. (That option is not set by default.) I need to see the script or other method with which you made this massive change to know whether w cane sure it did not introduce unintended and harmful changes. Don't put it back in CI, just attach a file with the relevant part of it to a comment in this PR.
I am not likely to need blame / annotate for future changes, as they will be reasonably fresh in my memory. And this will make it less convenient, though not impossible, for me to track past changes. I do realize that as people make changes to more files over time using editors that scrub trailing whitespace, I will have to use the less convenient workaround I developed last night, which I described above. I will leave it to the three of you, @AenBleidd @davidpanderson and @RichardHaselgrove to work out. In any case, i can't sped any more time on this this week. I hav other pressing matters. And I feel this has already been a major waste of my time. |
We should tell submitters to not combine functional changes with code cleanup. There are many forms of code cleanup. I have no objection to this PR, but I'd prefer to focus on stuff that matters. |
6226bc6
to
fe57bc8
Compare
Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
fe57bc8
to
4474202
Compare
I have double checked every file. It contains no changes except trailing whitespaces removal and script to do this verification. |
@AenBleidd I am glad to see that you did not remove whitespace from the dependent library build scripts in the mac_build directory (build curl.sh, etc.) Some of these contain embedded diffs used by patches in the script; these diffs must have a leading space on each line, including blank lines. Since the single space in the blank line id the entire line, it is also a trailing space; removing it will probably make the patch fail. Please be certain that the code in the CI does not remove these spaces in blank lines in these embedded diffs. Removing whitespaces in the CI script from these files will probably make their patches fail. |
@CharlieFenton, the code in the CI does verification only. It has an optional parameter that could be added when running this script locally to fix issues locally before making the PR. |
@AenBleidd That might avoid the problem for the moment, but someone could later set that flag without realizing that it can damage the dependent library build scripts. Please either modify the code to exclude the build scripts (preferred) or remove that capability. That could cause bugs that would be quite challenging to find. The patches installed by these build scripts are critical. Knowing that you are a careful and thoughtful programmer, I'm sure you don't want to leave a trap set for someone in the future due to an oversight in your code. There might be other places where removing trailing whitespace can cause problems, but this is the only one I know of now. |
@AenBleidd Please ignore my previous comment. I ran some tests and found that the patch utility works correctly even after removing the leading space in blank lines within the diff. My apologies. |
@CharlieFenton, you are right. I'll do that change. |
Ah, ok. 😀 |
No description provided.