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.

Sign up for GitHub

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

Jump to bottom

dev: Document astyle python script #6292

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

dev: Document astyle python script #6292

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion dev/source/docs/style-guide.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,15 @@ automatically according to the guidelines below.

astyle --options=Tools/CodeStyle/astylerc <filename>

Some files are enforced to be *astyle* clean. To format them automatically, run:
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we want to push users to run this all the time when only a few files are affected,,,,maybe we should specify that it should be checked if you have modified the following ..... libraries or files?

Copy link
Contributor

Choose a reason for hiding this comment

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

More maintenance burden, @Hwurzburg ?

We should probably put this advice - run astyle - into the CI error message when one of these files fails the checks, if we haven't already. It's much more likely to be useful there.

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 don't think the existing recommendation is useful because it

  • Doesn't tell you which file to use
  • None of the existing code is enforced to the style guide, so it would cause unrelated changes

Proposing a reformat of the codebase was rejected in dev call because it would make existing PR's and backports much more difficult to rebase. Thus, I would propose recommending making new files astyle clean, and then we document that in the wiki.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

what

* Doesn't tell you which file to use

you mean ? if you think that is an issue we could clarify what refers to

* None of the existing code is enforced to the style guide, so it would cause unrelated changes

its true when run on the file you have modified it would correct existing format issues....maybe we could start allowing an unsquashed ending commit in PRs for style changes to allow any new touched file to be astyle corrected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The devs have been very clear they do not want noise on PR's for style changes. What I am trying to say (not effectively I guess) is that the existing wiki instructions have no relation to our current development practices. From what I can tell, no one manually runs astyle on existing code using the method in the wiki.

I've proposed many options to improve the development practices and reduce the time developers spend nitpicking formatting. Hardly any of my proposals have been accepted by the development team, except for the python script which works wonderfully so far and the CI task for enforcing it.

Hence, I want to document it, because it's working well, and I truly hope other developers writing new libraries get on board with automatic formatting.


::

./Tools/scripts/run_astyle.py

Remember any formatting changes to a file should be done as a separate
commit.
commit. You can add formatting commits to the ``.git-blame-ignore-revs`` file
to not clutter history.

Case
====
Expand Down