Skip to content

Add new spacing rules pending from DML style guide #108

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
Merged
merged 13 commits into from
Oct 1, 2025
Merged

Add new spacing rules pending from DML style guide #108

merged 13 commits into from
Oct 1, 2025

Conversation

@jvsqzj
Copy link
Contributor

@jvsqzj jvsqzj commented May 29, 2025 •
edited
Loading

Address pending rules tracked in issue #71

  • SP.reserved: spaces around reserved words, such as if, else, default, size, const and in, except when a reserved word is used as an identifier (e.g., local uint8 *data;) - @alecalvop
  • SP.binop: spaces around binary operators except the dereferencing operators dot (a.b) and arrow (a->b) - @jvsqzj
  • SP.ternary: spaces around ? and : in the ternary ?: operator - @jvsqzj
  • SP.ptrdecl: space between a type and the * marking a pointer - @dtarassenko
  • NSP.ptrdecl: no space after the * marking a pointer in a declaration - @dtarassenko

@jvsqzj jvsqzj force-pushed the feature/71-spacing-pending-rules branch from e2b4518 to 4cc5936 Compare May 29, 2025 18:30
@jvsqzj jvsqzj mentioned this pull request May 29, 2025
Closed
@jvsqzj jvsqzj force-pushed the feature/71-spacing-pending-rules branch from 2af6f35 to 76b5474 Compare July 3, 2025 22:14
@jvsqzj
Copy link
Contributor Author

jvsqzj commented Jul 3, 2025

There seems to be a problem with the #[serde(default)] attribute we use for LintCfg and its fields.
If one forgets to add it, then the test_example_lintcfg will pass even if the example_lintcfg.json file does not have the added rule.

We should make this clear to avoid this type of silent issues.

@jvsqzj jvsqzj force-pushed the feature/71-spacing-pending-rules branch from f90a029 to 8b336dd Compare July 17, 2025 22:26
@jvsqzj
Copy link
Contributor Author

jvsqzj commented Aug 14, 2025

There seems to be a problem with the #[serde(default)] attribute we use for LintCfg and its fields. If one forgets to add it, then the test_example_lintcfg will pass even if the example_lintcfg.json file does not have the added rule.

We should make this clear to avoid this type of silent issues.

This is now handled by #130 and should be merged soon to resolve this problem

@jvsqzj jvsqzj marked this pull request as ready for review August 14, 2025 21:25
@jvsqzj jvsqzj force-pushed the feature/71-spacing-pending-rules branch from 8b18048 to 06c848a Compare August 14, 2025 21:27
@jvsqzj
Copy link
Contributor Author

jvsqzj commented Aug 14, 2025

@JonatanWaern I think it functionality here is complete. Please do a review round for feedback.

JonatanWaern
JonatanWaern reviewed Aug 15, 2025
View reviewed changes
Copy link
Contributor

@JonatanWaern JonatanWaern left a comment

Choose a reason for hiding this comment

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

Nice work! needs a few changes (see also clippy part of build) and a changelog entry

JonatanWaern
JonatanWaern reviewed Sep 8, 2025
View reviewed changes
@jvsqzj
Copy link
Contributor Author

jvsqzj commented Sep 22, 2025

Nice work! needs a few changes (see also clippy part of build) and a changelog entry

I think all suggestions are addressed now.

JonatanWaern
JonatanWaern reviewed Sep 26, 2025
View reviewed changes
Copy link
Contributor

@JonatanWaern JonatanWaern left a comment

Choose a reason for hiding this comment

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

Needs a rebase on main, which seems to have conflicts (likely due to my in-line lint annotation change, which also touches the tests on lint/mod.rs level.)

@JonatanWaern
Copy link
Contributor

JonatanWaern commented Sep 30, 2025

@alecalvop Are you looking to merge this soon? I was planning to make a release this week so we can likely squeeze it in before

alecalvop and others added 13 commits September 30, 2025 10:51
@alecalvop
Refactor lint spacing tests
97e8fe3 
Use robust check for spacing tests
@dtarassenko @alecalvop
Add pointer declaration spacing rule (SP.ptrdecl) (#20)
461b3eb 
* Add pointer declaration spacing rules

* Remove NspPtrDecl disable
@jvsqzj @alecalvop
Fix outdated push_err call and unit-tests
1400e63
@jvsqzj @alecalvop
Add sp_binop and sp_ternary rules to lint module checks (#19)
55793ba 
* Add sp_binop.rs to spacing rule tests module for BinaryExpressionContent
* Add sp_ternary rule for ? : conditionals for TertiaryExpressionContent
@alecalvop @jvsqzj
Add SP Reserved spacing rule (#21)
5f19219 
* Add SP Reserved spacing rule
* Add support for `while` `for` and `else`
* Add rule documentation
* Add rule to example_lint_cfg.json

---------

Co-authored-by: Vasquez Alfaro, Juan J <juan.j.vasquez.alfaro@intel.com>
@jvsqzj @alecalvop
Make name() return value lowercase for consistency
5d072b1
@dtarassenko @alecalvop
Add ptrdecl rules to supported features documentation
a679c88
@alecalvop
Misc consistency changes
9eb4fe5
@alecalvop
Fix clippy reports
732f0b7
@jvsqzj @alecalvop
Address code review suggestions
3552eba
@alecalvop
Update features.md
c0b1ab6
@alecalvop
Update SpTernaryRule reported ranges
0055369
@alecalvop
Add CHANGELOG entry
1db4874
@alecalvop alecalvop force-pushed the feature/71-spacing-pending-rules branch from a9d1b48 to 1db4874 Compare September 30, 2025 17:08
@alecalvop
Copy link
Contributor

alecalvop commented Sep 30, 2025

@alecalvop Are you looking to merge this soon? I was planning to make a release this week so we can likely squeeze it in before

@JonatanWaern Yes, I have addressed the comments

JonatanWaern
JonatanWaern approved these changes Oct 1, 2025
View reviewed changes
Copy link
Contributor

@JonatanWaern JonatanWaern left a comment

Choose a reason for hiding this comment

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

Looks good, nice work!

@JonatanWaern JonatanWaern merged commit a13c1d8 into intel:main Oct 1, 2025
4 checks passed
@alecalvop alecalvop mentioned this pull request Oct 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JonatanWaern JonatanWaern JonatanWaern approved these changes

+1 more reviewer

@alecalvop alecalvop alecalvop left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jvsqzj @JonatanWaern @alecalvop @dtarassenko