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

Inconsistency: Align or don't align type clauses? #289

Open
bjoern-jueliger-sap opened this issue Nov 3, 2022 · 1 comment
Open

Inconsistency: Align or don't align type clauses? #289

bjoern-jueliger-sap opened this issue Nov 3, 2022 · 1 comment
Labels
Clarity of Text The issue or pull request helps to improve the clarity of the text

Comments

@bjoern-jueliger-sap
Copy link
Member

bjoern-jueliger-sap commented Nov 3, 2022

The guide recommends not aligning the type clauses for different variables on the same level, with the clear reasoning that it is the type and the variable that belong together, not the two types that are being aligned.

However, the guide then does not follow this recommendation in both method declarations and structure type declarations, where the types clauses of the method parameter resp. structure component are being aligned.

This is inconsistent: Either the guide meant to say that the type clauses of unrelated variables should not be aligned and then the "Don't align type clauses" section should explicitly say that, or the guide should take its own advice and never align type clauses.

Personally I think we should get rid of all the useless whitespace and stop aligning type clauses and assignments altogether (other people in other languages think so, too: e.g. rustfmt, whose default configuration is the de facto style guide for the Rust community, deletes all whitespace of this kind), but either way a consistent recommendation in the guide would be better than the current ambiguity. (Or, if no consensus can be reached on what should be recommended, this rule should be removed entirely and left to individual taste)

@bjoern-jueliger-sap bjoern-jueliger-sap added the Clarity of Text The issue or pull request helps to improve the clarity of the text label Nov 3, 2022
@fabianlupa
Copy link
Contributor

My train of thought on this: Consistency is most important. Always use pretty printer. Pretty printer will align some things horizontally without you being able to turn that off. Therefore also align other things.

Pretty printer was even somewhat recently extended to horizontally align the parameters in functional method calls at the equal sign (like function modules and the statement based method call syntax). So I am not sure what the direction is. Ideally the tooling should take care of all this, like rustfmt, gofmt etc. all do. And then not aligning things and not having useless whitespace (that also introduces diffs in lines that weren't actually changed in version control) would in my opinion be preferable.

The example you mention first is manual alignment, while this would have been automatic with pretty printer if a chained statement was used (which is recommended against in the same chapter). You could apply the same logic to structure type declarations and do them without a chained statement but then the components will not be aligned at a deeper intendation level, which argueably does make sense and increase readability.

TYPES BEGIN OF structure.
TYPES component_a TYPE i.
TYPES component_b TYPE i.
TYPES BEGIN OF sub_structure.
TYPES sub_component_a TYPE i.
TYPES END OF sub_structure.
TYPES END OF structure.

TYPES: BEGIN OF structure,
         component_a TYPE i,
         component_b TYPE i,
         BEGIN OF sub_structure,
           sub_component_a TYPE i,
         END OF sub_structure,
       END OF structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clarity of Text The issue or pull request helps to improve the clarity of the text
Projects
None yet
Development

No branches or pull requests

2 participants