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

Path Configuration / Profile #8

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Path Configuration / Profile #8

merged 1 commit into from
Aug 26, 2024

Conversation

harvey0100
Copy link
Contributor

@harvey0100 harvey0100 commented Aug 2, 2024

Check-lint will now check for config file
avocado_static_checks.conf one directory above
as the submodule is placed into the root of the
directory. If the file is not present it will return back to the default method.

Reference: #5

@richtja richtja requested review from clebergnu and richtja August 2, 2024 09:26
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @harvey0100, thank you for this update, I heave few comments for you. And can you please create some example of how this should work, I wasn't able to run it on my system, and I am not sure how it should be done. Thank you.

@harvey0100
Copy link
Contributor Author

@clebergnu Hi Cleber as discussed yesterday, could you please leave feedback here in writing so I know fully what we would like to achieve?

@harvey0100
Copy link
Contributor Author

Updated, now uses default config against all files if the avocado conf does not exist.
If the path specified by user inside of the conf does not exist it exits with an error notifying the user.

Example config:

[lint]
autils:avocado-static-checks/default_configs/pylintrc
tests:avocado-static-checks/default_configs/pylintrc

Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @harvey0100, thank you for your update, IMO is definitely getting closer to the desired solution. Please add an example to the README file with explanation of this feature. Also, I did few tests and I have few examples which doesn't work as they should, please let me know what do you think about it.:

[lint]
autils:.pylintrc

If I have only this in .avocado_static_checks.conf the lint check will be only applied to autils directory and other files will be skipped. IMO it should apply the stati-checks/default_configs/pylintrc to other files in the repo.

[lint]
autil:.pylintrc

Here I made typo autil/autils and the static-checks were successful without any lint check, and I wasn't notified that autil path doesn't exist.

[black]
tests:tests/.pylintrc
[lint]
autils:.pylintrc

Here I tried if the [lint] tag is respected, but it seems to me that is not, because the tests:tests/.pylintrc were applied as well. IMO we have two options here how to solve this, we can update the algorithm to respect the tags, which will make it easier to add more checks in the future, or we can remove the [lint] tag for now. I would prefer the second option, because for now we don't have use-case for other checks.

Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @harvey0100, thanks for the updated, we are getting there. I have just one comment about CUSTOM_CONFIG_FILES tempfile. Please have a look and let me know what do you think.

@harvey0100
Copy link
Contributor Author

Hi @richtja looks like i did not push my fork correctly.
It should all be correct now with the changes i described such as an array.

Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @harvey0100, thanks for the update. Unfortunately, I found one more bug. When I put the empty line into the .avocado_static_checks.conf the script will break down.

[lint]

tests:tests/.pylintrc
autils:.pylintrc

output:

Running './static-checks/check-import-order'
** Running isort (/home/janrichter/.pyenv/shims/isort)...
Skipped 1 files
./static-checks/check-import-order PASSED


Running './static-checks/check-lint'
Found configuration file: ./static-checks/../avocado-static-checks.conf
Error: Config file '' specified for directory '' does not exist.

@harvey0100
Copy link
Contributor Author

Hi @richtja included a fix for empty lines now, thank you :)

Check-lint will now check for config file
avocado_static_checks.conf one directory above
as the submodule is placed into the root of the
directory. If Avocado static check conf does not exist
it will return to default config. Will use custom config on specified
directories. Remaining files with no custom config will also use default conf.
Header tags inside the static conf are now respected.

Reference:      #5
Signed-off-by:  <[email protected]>
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @harvey0100, thanks for the latest updates. It LGTM now.

Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Tested, work as expected! Thanks!

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

Successfully merging this pull request may close these issues.

3 participants