Allow glob patterns instead of parent dirs for matching configs #156

Kronuz · 2020-11-18T02:35:09Z

Summary

Allow using globs as configuration for rules, not only parent directories.

This allows for creating specific rules for specific files, not only directories. For example:

{
    "*/urls.py": {
        "rules": [
            ["*", "deny"]
        ]
    }
}

Will allow specifying rules for all urls modules anywhere.

This also allows specifying if the allow or deny rule are meant for global or local scopes only:

{
    "*/urls.py": {
        "rules": [
            ["common.*", "allow"],
            ["*", "deny global"]
        ],
        "message": "Module in {current_file} only allows certain global imports. {imported} cannot be imported here."
    }
}

Allowing to import common.* anywhere and everything else only inside functions, as inner imports.

Test Plan

Run tests

jimmylai

There are lint errors.

fixit/rules/import_constraints.py

jimmylai · 2020-11-19T03:57:43Z

The CI builds has a test case failure.
https://github.com/Instagram/Fixit/pull/156/checks?check_run_id=1419755011

It looks like a bug in the new version of LibCST related to the new Assignment/Access changes. CC @zsol

It changes b = [a for a in [1,2,3]] as b = [cls for a in [1,2,3]] which is wrong. The local access of a in the comprehension should refer to the local definition of var a in the comprehension.

jimmylai · 2020-12-01T23:26:34Z

@Kronuz there is a type check error regarding fnmatch.
https://github.com/Instagram/Fixit/pull/156/checks?check_run_id=1483019512

codecov-io · 2020-12-02T00:06:43Z

Codecov Report

Merging #156 (93e1c49) into master (6e662b2) will increase coverage by 0.08%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   84.71%   84.80%   +0.08%     
==========================================
  Files          86       86              
  Lines        3532     3566      +34     
==========================================
+ Hits         2992     3024      +32     
- Misses        540      542       +2

Impacted Files	Coverage Δ
fixit/common/testing.py	`92.30% <50.00%> (-0.96%)`	⬇️
fixit/rules/import_constraints.py	`92.52% <97.61%> (+0.92%)`	⬆️
fixit/common/utils.py	`92.03% <100.00%> (+0.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e662b2...93e1c49. Read the comment docs.

jimmylai · 2020-12-02T06:53:16Z

fixit/rules/import_constraints.py

+class Action(Enum):
+    ALLOW = "allow"  # allow everywhere
+    DENY = "deny"  # deny everywhere
+    ALLOW_GLOBAL = "allow_global"  # allow in global scope
+    ALLOW_LOCAL = "allow_local"  # allow in local scopes
+    DENY_GLOBAL = "deny_global"  # deny in global scope
+    DENY_LOCAL = "deny_local"  # deny in local scopes
+
+
+ACTIONS_MAP: Dict[str, Action] = {
+    "allow": Action.ALLOW,
+    "deny": Action.DENY,
+    "allow global": Action.ALLOW_GLOBAL,
+    "allow local": Action.ALLOW_LOCAL,
+    "deny global": Action.DENY_GLOBAL,
+    "deny local": Action.DENY_LOCAL,
+    # Aliases:
+    "allow globally": Action.ALLOW_GLOBAL,
+    "allow locally": Action.ALLOW_LOCAL,
+    "deny globally": Action.DENY_GLOBAL,
+    "deny locally": Action.DENY_LOCAL,
+}


ACTIONS_MAP looks duplicated.
To check a string is a valid Enum class attribute, just do action in Action.__members__.
To get the enum value from a string, simply do Action[action]

Suggested change

class Action(Enum):

ALLOW = "allow" # allow everywhere

DENY = "deny" # deny everywhere

ALLOW_GLOBAL = "allow_global" # allow in global scope

ALLOW_LOCAL = "allow_local" # allow in local scopes

DENY_GLOBAL = "deny_global" # deny in global scope

DENY_LOCAL = "deny_local" # deny in local scopes

ACTIONS_MAP: Dict[str, Action] = {

"allow": Action.ALLOW,

"deny": Action.DENY,

"allow global": Action.ALLOW_GLOBAL,

"allow local": Action.ALLOW_LOCAL,

"deny global": Action.DENY_GLOBAL,

"deny local": Action.DENY_LOCAL,

# Aliases:

"allow globally": Action.ALLOW_GLOBAL,

"allow locally": Action.ALLOW_LOCAL,

"deny globally": Action.DENY_GLOBAL,

"deny locally": Action.DENY_LOCAL,

}

class Action(Enum):

ALLOW = "allow" # allow everywhere

DENY = "deny" # deny everywhere

ALLOW_GLOBAL = "allow_global" # allow in global scope

ALLOW_LOCAL = "allow_local" # allow in local scopes

DENY_GLOBAL = "deny_global" # deny in global scope

DENY_LOCAL = "deny_local" # deny in local scopes

Yes, the idea behind the map was to also allow other aliases (such as "allow globally", "allow global", and "allow_global"). But yes, we can use the simple Enum, if we don't want that support.

Not supporting different alias doesn't seem to be a big issue. It can also keep the config more consistent.

jimmylai · 2020-12-02T06:53:49Z

fixit/rules/import_constraints.py

+            if action not in ACTIONS_MAP:
+                raise ValueError(
+                    "A rule should either allow or deny a pattern (possibly globally or locally)"
+                )
+            return _ImportRule(module, ACTIONS_MAP[action])


Suggested change

if action not in ACTIONS_MAP:

raise ValueError(

"A rule should either allow or deny a pattern (possibly globally or locally)"

)

return _ImportRule(module, ACTIONS_MAP[action])

if action not in Action.__members__:

raise ValueError(

f"A rule should have a valid action ({list(Action.__members__)})"

)

return _ImportRule(module, Action[action])

We could make this whole block even simpler if we don't want that map for aliases, albeit with a more generic error (ValueError: 'invalid_action' is not a valid RuleAction):

return _ImportRule(module, Action(action))

fixit/rules/import_constraints.py

jimmylai · 2020-12-02T07:16:12Z

fixit/common/testing.py

+            if test_case.message is not None and test_case.message != report.message:
+                raise AssertionError(
+                    f"Expected message:\n    {test_case.message}\nBut got:\n    {report.message}"
+                )
+


The message in InvalidTestCase is only used in one of the new test case. Do we really need to add this new field?
Ideally, the test case example code should explain itself clearly. The example code can also have some comments to explain it better. I'd suggest not to add InvalidTestCase.message to keep it simpler.

I wanted to test the custom messages are working, not only show they exist. This allows testing custom messages and it doesn't add much complexity, only a new field that can be tested. we could call the field "expected_message" though. What do you think?

Yeah, expected_message is a better name. I can see it can be useful for rules provide custom messages.

…r global scopes

jimmylai

LGTM
When releasing this, we need to update existing config file to avoid the new RuleAction value error.

Kronuz · 2020-12-03T00:56:09Z

LGTM
When releasing this, we need to update existing config file to avoid the new RuleAction value error.

What do you mean?

jimmylai · 2020-12-03T01:06:10Z

LGTM
When releasing this, we need to update existing config file to avoid the new RuleAction value error.

What do you mean?

I read it wrong, so there won't be value error since we keep the lower case allow/deny. We can review existing rule to see if we need to refine them.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 18, 2020

jimmylai reviewed Nov 18, 2020

View reviewed changes

fixit/rules/import_constraints.py Show resolved Hide resolved

Kronuz force-pushed the Kronuz-globs branch 2 times, most recently from ae2c12e to 2651106 Compare November 18, 2020 17:52

zsol mentioned this pull request Nov 18, 2020

Allow files to be excluded for AddMissingHeaderRule #157

Closed

jimmylai reviewed Nov 19, 2020

View reviewed changes

fixit/rules/import_constraints.py Outdated Show resolved Hide resolved

Kronuz force-pushed the Kronuz-globs branch from eabb4e3 to ec8e80b Compare November 20, 2020 01:04

Kronuz force-pushed the Kronuz-globs branch 2 times, most recently from 2651106 to 06cd4ac Compare December 1, 2020 22:37

Allow glob patterns instead of parent dirs for matching configs

efef420

Kronuz force-pushed the Kronuz-globs branch 3 times, most recently from 6f6b652 to d413093 Compare December 2, 2020 00:38

Kronuz requested a review from jimmylai December 2, 2020 00:49

jimmylai reviewed Dec 2, 2020

View reviewed changes

Kronuz force-pushed the Kronuz-globs branch from d413093 to 2331d95 Compare December 2, 2020 22:31

Added support for allowing or denying imports specifically in local o…

7b5b0a9

…r global scopes

Kronuz force-pushed the Kronuz-globs branch from 2331d95 to 7b5b0a9 Compare December 2, 2020 23:32

jimmylai approved these changes Dec 3, 2020

View reviewed changes

Kronuz merged commit b3624f5 into master Dec 3, 2020

lpetre deleted the Kronuz-globs branch January 6, 2022 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow glob patterns instead of parent dirs for matching configs #156

Allow glob patterns instead of parent dirs for matching configs #156

Kronuz commented Nov 18, 2020 •

edited

Loading

jimmylai left a comment

jimmylai commented Nov 19, 2020

jimmylai commented Dec 1, 2020

codecov-io commented Dec 2, 2020 •

edited

Loading

jimmylai Dec 2, 2020

Kronuz Dec 2, 2020

jimmylai Dec 3, 2020

jimmylai Dec 2, 2020

Kronuz Dec 2, 2020

jimmylai Dec 2, 2020

Kronuz Dec 2, 2020

jimmylai Dec 3, 2020

jimmylai left a comment

Kronuz commented Dec 3, 2020

jimmylai commented Dec 3, 2020

Allow glob patterns instead of parent dirs for matching configs #156

Allow glob patterns instead of parent dirs for matching configs #156

Conversation

Kronuz commented Nov 18, 2020 • edited Loading

Summary

Test Plan

jimmylai left a comment

Choose a reason for hiding this comment

jimmylai commented Nov 19, 2020

jimmylai commented Dec 1, 2020

codecov-io commented Dec 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylai left a comment

Choose a reason for hiding this comment

Kronuz commented Dec 3, 2020

jimmylai commented Dec 3, 2020

Kronuz commented Nov 18, 2020 •

edited

Loading

codecov-io commented Dec 2, 2020 •

edited

Loading