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

No way to incrementally improve linters #233

Open
rotu opened this issue Apr 29, 2020 · 1 comment
Open

No way to incrementally improve linters #233

rotu opened this issue Apr 29, 2020 · 1 comment
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@rotu
Copy link
Contributor

rotu commented Apr 29, 2020

As discovered when doing #174, the expectation of making linter changes is that, even when fixing linters to comply with documented coding standards, there is an expectation of wide-ranging PRs in parallel to accommodate these changes.

It seems we need a better path forward for linters already in use. Some discussion here: ros2/rclcpp#1052

Here are some suggested fixes:
(1) allow specifying a reference commit so style issues before that commit are grandfathered in, preventing code churn.
(2) version the coding standards so that packages have to affirmatively opt in to later refinements, and they don't automatically create CI regressions.

@rotu
Copy link
Contributor Author

rotu commented Apr 29, 2020

Relevant feedback from ros2/rclcpp#1052:

@ivanpauno:

This PR has some “examples of the proposed changes” as requested in ament/ament_lint#174 (comment)

I think that having an example is great, and the ament_lint issue still references this PR.

It seems fixing ament_lint is an uphill battle, and there needs to be a better way than a billion PRs more or less simultaneously. Maybe ament_uncrustify should only check changed code to prevent this sort of “churny” PR. Or maybe we should version the checking rules so that uncrustify and consuming packages don’t need to move in lock-step.

The disadvantage of that is that we will have dissimilar styles in different packages.
Though doing the change in a lot of packages at the same time isn't nice, I think it's the correct thing to do if we want to update the style.

@dirk-thomas:

Maybe ament_uncrustify should only check changed code to prevent this sort of “churny” PR.

The concept of "changed code" doesn't exist when you e.g. run the tests for a workspace since there is not reference point to determine a change against.

Or maybe we should version the checking rules so that uncrustify and consuming packages don’t need to move in lock-step.

Configuration files could be versioned and downstream packages could choose a specific version. That is already technically possible since you can pass a specific config file to the linter. But as a matter of fact all existing packages use the default config and therefore would still need to be updated at least one.

Also as soon as the uncrustify version is changes there will likely be breaking changes. So you would need to ship different uncrustify versions in parallel.

If you are interested in either of these ideas please consider to contribute towards this (with pull requests and if the changes are more complex consider to start with a design issue to discuss the proposal).

@hidmic hidmic added enhancement New feature or request help wanted Extra attention is needed labels May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants