Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add config linting script that checks for bool casing #6203

Merged
merged 12 commits into from
Oct 23, 2019

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Oct 12, 2019

Add a linting script that enforces all boolean values in the default config be lowercase.

This has annoyed me for a while so I decided to fix it.

Pipelines repo PR: matrix-org/pipelines#12 Not necessary anymore.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

great idea, couple of tweaks though

@@ -0,0 +1,9 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

it might be simpler just to grep for the incorrect formats, rather than try to correct them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this at first, but I wanted to be able to add this to scripts-dev/lint.sh which could get run in a pre-commit hook for instance.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure that follows... lint.sh doesn't correct for incorrect imports or pep8 failures, it just complains if they are wrong.

There might be value in having a script to correct the problems, but given how often it's likely to be a problem versus the added complexity of this script, I don't think it's worthwhile?

# Exits with 0 if there are no problems, or another code otherwise.

# Fix non-lowercase true/false values
sed -i -E "s/(#.*): +True(.*)/\1: true\2/g; s/(#.*): +False(.*)/\1: false\2/g;" synapse/config/*.py
Copy link
Member

Choose a reason for hiding this comment

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

  1. why do we require a #
  2. some of the groups are redundant:
Suggested change
sed -i -E "s/(#.*): +True(.*)/\1: true\2/g; s/(#.*): +False(.*)/\1: false\2/g;" synapse/config/*.py
sed -i -E "s/(#.*): +True/\1: true/g; s/(#.*): +False/\1: false/g;" synapse/config/*.py

Copy link
Member Author

Choose a reason for hiding this comment

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

The code can contain false positives without the hash, such as docstrings:

use_worker_options (bool): True to use the 'worker_log_config' option

Copy link
Member

Choose a reason for hiding this comment

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

ahhh. we should probably run it on the sample config rather than attempt to match only the config bits within synapse/config.

tox.ini Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 requested a review from richvdh October 21, 2019 12:19
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I think it's worth noting that your script has missed

enabled: False
.

@anoadragon453
Copy link
Member Author

I think it's worth noting that your script has missed

enabled: False

.

Yep, had to fix this case manually. I've also switched it so that the script just fixes the sample config instead of your code. I also changed the git diff to only check the sample config, so that it doesn't print out any changes you've made otherwise.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm then

@anoadragon453 anoadragon453 merged commit 409c62b into develop Oct 23, 2019
@anoadragon453 anoadragon453 deleted the anoa/lowercase_config_defaults branch October 23, 2019 12:22
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '409c62b27':
  Add config linting script that checks for bool casing (#6203)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants