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

Updated configs to use list format for 'inherits_from' #427

Merged
merged 14 commits into from
Aug 1, 2022

Conversation

gregtkogut
Copy link
Collaborator

No description provided.

@gregtkogut gregtkogut self-assigned this Jun 30, 2022
@tdenewiler
Copy link
Contributor

I got the unit tests to pass with the following diff.

diff --git a/statick_tool/config.py b/statick_tool/config.py
index 2f207fd..a6fd41f 100644
--- a/statick_tool/config.py
+++ b/statick_tool/config.py
@@ -135,11 +135,12 @@ class Config:
                 )
             configs = ""
             for inherited_level in self.config["levels"][level]["inherits_from"]:
-                config = self.get_plugin_config(
-                    plugin_type, plugin, inherited_level, key, default
-                )
-                if config is not None:
-                    configs += config
+                if level is not inherited_level:
+                    config = self.get_plugin_config(
+                        plugin_type, plugin, inherited_level, key, default
+                    )
+                    if config is not None:
+                        configs += config
             if configs:
                 return configs
         return default

@denewiler
Copy link
Collaborator

Looks like you need to merge in main again. Main now has your fix to the mypy issue causing workflow issues in this PR.

@gregtkogut gregtkogut requested a review from denewiler July 26, 2022 14:59
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #427 (55f0f72) into main (b8431ad) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #427   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           57        57           
  Lines         3286      3284    -2     
=========================================
- Hits          3286      3284    -2     
Impacted Files Coverage Δ
statick_tool/config.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@gregtkogut gregtkogut marked this pull request as draft July 26, 2022 15:08
@gregtkogut
Copy link
Collaborator Author

@denewiler, your diff caused a drop in coverage...I'll take a look at that later, but feel free to push another diff if you feel you can do it quicker, being more familiar with the testing.

@gregtkogut
Copy link
Collaborator Author

Unfortunately every time I revisit this there's something new broken....any insight with the tox failure? I ran the the Statick check on the main branch too, and get the same thing, so it's probably unrelated to this PR.

@tdenewiler
Copy link
Contributor

The tox errors are new for the main branch. I'll take a look. Note that there is an open PR that fixes coverage for the update_inherits branch. The PR is at gregtkogut#1 and is waiting on permission for actions to run.

@tdenewiler
Copy link
Contributor

Found the culprit at tholo/pytest-flake8#87. The upgrade to flake8 v5 broke pytest-flake8 to give the error we are seeing.

AttributeError: module 'flake8.options.config' has no attribute 'ConfigFileFinder'

I have a one-line fix working in Windows, will PR soon if Actions pass.

@gregtkogut
Copy link
Collaborator Author

The tox errors are new for the main branch. I'll take a look. Note that there is an open PR that fixes coverage for the update_inherits branch. The PR is at gregtkogut#1 and is waiting on permission for actions to run.

Thanks, hadn't seen that PR. Waiting on your 1-line fix to pass the Action for both that PR and then eventually this one.

@gregtkogut gregtkogut marked this pull request as ready for review August 1, 2022 02:14
@gregtkogut
Copy link
Collaborator Author

Thanks for the help, I think this is finally ready.

@denewiler denewiler merged commit c8fd2b8 into sscpac:main Aug 1, 2022
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.

Default configs do not use suggested new list format for "inherits_from"
3 participants