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

Global: apply guidelines #6497

Closed
wants to merge 1 commit into from
Closed

Global: apply guidelines #6497

wants to merge 1 commit into from

Conversation

0penBrain
Copy link
Contributor

@0penBrain 0penBrain commented Mar 2, 2022

find ./ -iname "*.cpp" -not -path "./src/3rdParty/*" \( -exec perl -p -e 's/(^[[:space:]]*)(if[\ \(](?:(?!\/[\/\*]).)*?)[[:space:]]*(return(?:\;(?!\\)|\ .*?(?!\\)\;(?!\\)))((?:(?!\\)[[:space:]]*\/[\/\*].*))?/\1\2\4\n\1    \3/g' -i {} \; -exec perl -p -e 's/^([[:space:]]*?)([[:space:]]{4})(return[\ \;][^\{]*?(?:\/\*.*\*\/)?[^\{]*?)[[:space:]]*(\}.*)/\1\2\3\n\1\4/g' -i {} \; \)

Only applied on cpp files excluding 3rd party

@github-actions github-actions bot added Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB Draft Related to the Draft Workbench WB FEM Related to the FEM Workbench WB Mesh Related to the Mesh Workbench WB Part Related to the Part Workbench WB Part Design Related to the Part Design Workbench WB CAM Related to the CAM/Path Workbench WB Points Related to the Points Workbench WB Sketcher Related to the Sketcher Workbench WB Spreadsheet Related to the Spreadsheet Workbench WB TechDraw Related to the TechDraw Workbench labels Mar 2, 2022
@freecadci
Copy link

pipeline status for feature branch PR_6497. Pipeline 483331890 was triggered at 84e8d9c. All CI branches and pipelines.

@yorikvanhavre
Copy link
Member

IMHO this kind of large cleaning operation should be postponed to after the 0.20 release...

@0penBrain
Copy link
Contributor Author

Up to you. But that's at no risk as it's easy to check that compiled binaries are same before and after. ;)

@luzpaz
Copy link
Contributor

luzpaz commented Mar 4, 2022

When the time does come and if this approach is approved, IMHO consider breaking the PR up in to several commits categorized by the directory tree where each commit addresses a different part of the ./src/ hierarchy.

@chennes
Copy link
Member

chennes commented Mar 6, 2022

The only downside to this type of cleanup is that it's going to introduce a bunch of conflicts for anyone working on these chunks of code. They are trivial to resolve, but still some work for authors. I propose that we do this for 0.20, but do it last, after reviewing all of the other PRs we intend to merge. As @0penBrain mentions, we can easily verify that the compiled binaries are identical, so there is no danger of introducing regressions. And the provided shell script can simply be run anew by one of us, without actually having to rebase this PR (that is, we technically ignore the PR itself, but directly apply the script).

@donovaly
Copy link
Member

I propose that we do this for 0.20, but do it last

I agree. This is the same as with PR #4556

@chennes , can I give this task in your hands?

@donovaly donovaly added this to the 0.20 milestone Mar 21, 2022
@chennes
Copy link
Member

chennes commented Mar 21, 2022

Yes, I can do both this and #4556 when you give me the go-ahead that we are ready for it.

@donovaly
Copy link
Member

Yes, I can do both this and #4556 when you give me the go-ahead that we are ready for it.

Many thanks!
The plan is to close the merging window by end of the coming Sunday. So on Monday please act.

Then we still have time to check if everything works and small bugfix PRs won't be affected/can be updated if necessary.

@luzpaz
Copy link
Contributor

luzpaz commented Mar 21, 2022

Since we're about to run the code over with a tractor 😜 might as well do #5578 as well ?

@donovaly
Copy link
Member

Since we're about to run the code over with a tractor 😜 might as well do #5578 as well ?

This needs to be discussed first. Can you please open a thread in the developer's forum?

@luzpaz
Copy link
Contributor

luzpaz commented Mar 21, 2022

@donovaly here's the original thread https://forum.freecadweb.org/viewtopic.php?t=19866

@donovaly
Copy link
Member

@donovaly here's the original thread https://forum.freecadweb.org/viewtopic.php?t=19866

Thanks. The question if this is still relevant. The thread is 5 years old and meanwhile Git's standard is to commit as is. I am on Windows and here I only get issues when a file has mixed line endings. Bit by bit there files go away.
You can open a new thread to discuss if handling the line endings is still an issue.

@adrianinsaval
Copy link
Member

Aren't the mixed line endings exactly what is trying to be solved? Anyways, would it be a good idea to include this PR's commit in the .git-blame-ignore-revs file? As it doesn't really change the intent of any of the code, just the formatting.

@chennes
Copy link
Member

chennes commented Mar 22, 2022

would it be a good idea to include this PR's commit in the .git-blame-ignore-revs file? As it doesn't really change the intent of any of the code, just the formatting.

Yes, IMO -- it is my intention to do so.

@donovaly
Copy link
Member

@chennes , will you do this today or later?

@chennes
Copy link
Member

chennes commented Mar 28, 2022

I'll take care of it later this evening, things got busy at work.

@chennes
Copy link
Member

chennes commented Mar 29, 2022

@0penBrain when I run your script, I am not seeing proper indentation on the return lines -- they are only indented a single space more than the if, rather than four. This happens whether I run it on Windows or Linux. Can you investigate?

@0penBrain
Copy link
Contributor Author

@chennes we'll look at this. Maybe it has to wait for tomorrow. :)

@0penBrain
Copy link
Contributor Author

0penBrain commented Mar 29, 2022

OK, see the problem. The snippet is correct when I edit it in the PR description, but isn't when it's displayed or quoted.
There shall be 4 spaces between '\1' and '\3' (there is only one place where those 2 follow each other -- with space in between --, tell me if you can't find it).

@0penBrain
Copy link
Contributor Author

@chennes should be fixed in the OP of the PR.
I used a different quoting that seems to better preserve spaces.

@chennes
Copy link
Member

chennes commented Mar 29, 2022

Applied as:
70488c8 Git: PR6497 Ignore commits in git blame
3ef6811 Web: PR6497 move return statement to new line
c44ccc0 TD: PR6497 move return statement to new line
cb21ac7 Spreadsheet: PR6497 move return statement to new line
1ee0467 Sketcher: PR6497 move return statement to new line
190d64b Sandbox: PR6497 move return statement to new line
ec74d35 Robot: PR6497 move return statement to new line
1e9236a Raytracing: PR6497 move return statement to new line
bac451c Points: PR6497 move return statement to new line
5f31fb7 Path: PR6497 move return statement to new line
6f230d7 PD: PR6497 move return statement to new line
0a65575 Part: PR6497 move return statement to new line
702238a Mesh: PR6497 move return statement to new line
35156cd Import: PR6497 move return statement to new line
bb0e2f1 FEM: PR6497 move return statement to new line
b130886 Draft: PR6497 move return statement to new line
1ea3643 Tools: PR6497 move return statement to new line
3b42d4a Base: PR6497 move return statement to new line
bedf920 Gui: PR6497 move return statement to new line

@chennes chennes closed this Mar 29, 2022
luzpaz added a commit to luzpaz/FreeCAD that referenced this pull request Dec 3, 2022
luzpaz added a commit to luzpaz/FreeCAD that referenced this pull request Dec 4, 2022
luzpaz added a commit to luzpaz/FreeCAD that referenced this pull request Dec 6, 2022
0penBrain pushed a commit that referenced this pull request Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB CAM Related to the CAM/Path Workbench WB Draft Related to the Draft Workbench WB FEM Related to the FEM Workbench WB Mesh Related to the Mesh Workbench WB Part Design Related to the Part Design Workbench WB Part Related to the Part Workbench WB Points Related to the Points Workbench WB Sketcher Related to the Sketcher Workbench WB Spreadsheet Related to the Spreadsheet Workbench WB TechDraw Related to the TechDraw Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants