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

Fix style issues from editorconfig configuration #4966

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

hiddewie
Copy link
Contributor

@hiddewie hiddewie commented May 10, 2024

Ref #4965 (comment)

Indicated by @pnorman to be put into a separate pull request.

This pull request fixes formatting issues compared to the configured editorconfig.

@hiddewie hiddewie marked this pull request as ready for review May 10, 2024 19:31
@hiddewie hiddewie changed the title Remove trailing whitespace from styles/*.mss files Remove trailing whitespace from styles and project files May 10, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@hiddewie hiddewie changed the title Remove trailing whitespace from styles and project files Configure and fix linting from editorconfig configuration May 11, 2024
@hiddewie
Copy link
Contributor Author

Looks better now. The linter found more issues with the scripts / docs / code, which I fixed. I disabled the rule to check for indent sizes matching the editorconfig, because there were too many errors that did not seem like big issues (indenting in SQL queries for example).

@hiddewie hiddewie requested a review from pnorman May 11, 2024 09:00
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

I am fine with removing end-of-line whitespace from files and making formatting more consistent - although it is worth to consider doing this after we finish changes currently in the works with more extensive changes to the code (#4952, #4965).

I am not ok with elevating a file that has been introduced as an optional helper for developers to binding policy without discussion.

I am generally wary about CI tools generating cryptic error messages without guidance how to fix or documentation on the underlying principles. This is, unfortunately, common in open source projects meanwhile and de facto (intentionally or not) discourages/excludes less technically minded people from contributing. We should not do that. If we want CI to enforce cosmetic coding requirements it should generate clearly worded messages about (a) what is wrong and (b) how to fix it.

@hiddewie
Copy link
Contributor Author

I am generally wary about CI tools generating cryptic error messages without guidance how to fix or documentation on the underlying principles.

For me the messages in https://github.com/gravitystorm/openstreetmap-carto/actions/runs/9042451722/job/24848982515#step:17:54 are pretty clear, annotated with the file, the line number and the problem.

This is, unfortunately, common in open source projects meanwhile and de facto (intentionally or not) discourages/excludes less technically minded people from contributing. We should not do that.

To be honest I prefer CI to tell me what is wrong with my PR instead of relying on a human review to verify and correct any cosmetic style changes. Especially for open source projects where maintainer time is scarce, and that time is better spent on validating things only humans can validate in a pull request.

I am not ok with elevating a file that has been introduced as an optional helper for developers to binding policy without discussion.

OK, I will remove all enforcement of the style changes from this PR. The maintainers can have a discussion about introducing such checks in CI.

@hiddewie hiddewie requested a review from imagico May 11, 2024 12:30
@hiddewie hiddewie changed the title Configure and fix linting from editorconfig configuration Fix style issues from editorconfig configuration May 11, 2024
@imagico
Copy link
Collaborator

imagico commented May 11, 2024

I am generally wary about CI tools generating cryptic error messages without guidance how to fix or documentation on the underlying principles.

For me the messages in https://github.com/gravitystorm/openstreetmap-carto/actions/runs/9042451722/job/24848982515#step:17:54 are pretty clear, annotated with the file, the line number and the problem.

I admit i have seen much worse in that regard. But still this is not exactly inviting for contributors, especially not if a minor formatting issue in a markdown file is shown as an error just like a hard coding error preventing the style from working.

Also curious that you need to allow external content from windows.net to get the detailed information (wtf?).

This is, unfortunately, common in open source projects meanwhile and de facto (intentionally or not) discourages/excludes less technically minded people from contributing. We should not do that.

To be honest I prefer CI to tell me what is wrong with my PR instead of relying on a human review to verify and correct any cosmetic style changes. Especially for open source projects where maintainer time is scarce, and that time is better spent on validating things only humans can validate in a pull request.

To be clear - i am not at all against using automated tools to help developers improve their PRs. I am against deploying those tools as a gatekeeper discouraging contributions by indicating them to be faulty possibly just because some indention inconsistency in a documentation file. If we can reduce the formatting things to a warning or info level instead of an error i would be happy including that.

Personally i think formal whitespace issues are not a very important aspect and it would be much more valuable to establish some more regular code maintenance works that looks for higher level inconsistencies (which are not caught by the tools discussed here).

@hiddewie
Copy link
Contributor Author

True.

This PR is mostly meant as a time saver for both people making PRs and reviewing them: making PRs is difficult because editors that honour the .editorconfig file will make changes to files (like strip whitespace, and end a file with a newline). These changes should not be committed in PRs that are not about that topic. Locally this means the contributor has changes that should not be committed. And if the changes are committed by accident, reviewers get busy reviewing unintended changes that should not been committed (e.g. #4965 (comment)).

All that time is better spent on changes that matter instead of discussing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants