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.

Sign up for GitHub

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

Jump to bottom

add a colon_prefixed_map option to Option to handle the per-file-ignores - [closed] #898

Closed
asottile opened this issue Apr 3, 2021 · 36 comments
Closed

add a colon_prefixed_map option to Option to handle the per-file-ignores - [closed] #898

asottile opened this issue Apr 3, 2021 · 36 comments
Labels
gitlab merge request

Comments

@asottile
Copy link
Member

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 14:41

Merges per-file-ignores -> master

Add a configuration option to Option for colon_prefixed_map. The accepted value is a YAML-style mapping but with only one depth. When True, the normalize method will return a Mapping.
@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 14:48

added 1 commit

  • f91c6bf - add a missing element to the `test_add_option_with_custom_args` test

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @codecov on Nov 6, 2018, 14:49

Codecov Report

Merging #265 into master will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   71.51%   71.75%   +0.23%     
==========================================
  Files          27       27              
  Lines        2398     2418      +20     
  Branches      390      401      +11     
==========================================
+ Hits         1715     1735      +20     
  Misses        604      604              
  Partials       79       79
Impacted Files Coverage Δ
src/flake8/main/options.py 100% <ø> (ø) ⬆️
src/flake8/style_guide.py 93.62% <100%> (+0.03%) ⬆️
src/flake8/options/manager.py 91.17% <100%> (+0.47%) ⬆️
src/flake8/utils.py 77.08% <100%> (+2.27%) ⬆️

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 4439ea2...c52e853. Read the comment docs.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 6, 2018, 14:51

Commented on src/flake8/options/manager.py line 155

how about:

value = {
    util.normalize_paths(key): value
    for key, v in value
}

right now you're (~kinda) mutating a mapping as you iterate and it's a little difficult to follow

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 6, 2018, 14:51

Commented on src/flake8/options/manager.py line 162

sometimes Tuple[str, ...] sometimes str irks me a bit -- should this be tuple on both sides?

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 6, 2018, 14:51

Commented on src/flake8/utils.py line 43

I'd like to see some unit tests of this function

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 6, 2018, 14:51

Commented on tests/unit/test_merged_config_parser.py line 241

hah, I wish we applied black to the tests. this call structure is so hard to maintain

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 6, 2018, 14:51

Commented on tests/unit/test_merged_config_parser.py line 236

no need to use parametrize when there's only one value

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 6, 2018, 14:52

I think this suffers the same problem as !263 -- it breaks the commandline usage?

Also could you create an issue, I think I know what problem you're trying to solve but it's not explicitly stated anywhere. It would be good to capture that in an issue :)

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 6, 2018, 14:58

Yes, please. I have no clue why this is being made and it tempts me to just close it.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 15:06

It's nice friendly reactions like this that keep people from submitting patches to open source projects.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 6, 2018, 15:32

@demosdemon Thanks for linking the issue! That helps understand the context and I definitely think that's a problem that should get fixed and this patch starts in the right direction for getting that fixed 👍

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 15:53

Commented on src/flake8/options/manager.py line 155

changed this line in version 3 of the diff

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 15:53

Commented on src/flake8/options/manager.py line 162

changed this line in version 3 of the diff

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 15:53

Commented on tests/unit/test_merged_config_parser.py line 241

changed this line in version 3 of the diff

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 15:53

Commented on tests/unit/test_merged_config_parser.py line 236

changed this line in version 3 of the diff

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 15:53

added 2 commits

  • e688a46 - resolve two discussion points
  • 6eca48b - convert complex key handling to a dict comprehension

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 15:57

added 1 commit

  • 4a17fe9 - update test to match other changes

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 16:01

added 1 commit

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 16:14

added 1 commit

  • f66e882 - add tests for `parse_colon_prefixed_map`

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 16:15

resolved all discussions

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 16:24

added 1 commit

  • 6aac8b9 - add docstring to test and another butt-ugly test

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 16:33

added 1 commit

  • 05e0ff3 - add test for when colon_prefixed_map=True and comma_separated_list=False and normalize_paths=False

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 16:35

diff coverage is at 💯

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 6, 2018, 16:52

Commented on src/flake8/utils.py line 45

I'd rather not reference YAML as part of the docstring. The format has nothing to do with YAML. Any similarities are purely coincidental.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 6, 2018, 16:52

Commented on src/flake8/main/options.py line 150

Can you explain the naming here? It doesn't make much sense to me. The colon isn't prefixing anything. It's infix if you want to describe it as an operator of sorts. What if we simplified this as parse_value_as_map

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 16:58

Commented on src/flake8/main/options.py line 150

naming things is hard, I didn't like colon_prefixed_map but it was the best I could think in the declarative style. parse_value_as_map is fine.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 17:12

added 1 commit

  • 00a6a63 - get styleguide to work harmoniously with per-file-ignores changes

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 17:18

Commented on src/flake8/utils.py line 45

changed this line in version 10 of the diff

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 17:18

added 1 commit

  • c52e853 - adapt the test to accomodate for unordered Mappings

Compare with previous version

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @demosdemon on Nov 6, 2018, 19:19

I've rethought the whole thing and wrote a much more in-depth test to cover a lot of cases.

def test_parse_key_value_pairs():
    """Test the key value parser for various edge cases."""
    string = '''\
        test.py:E123
        # comment and an extra space between the colon and the errorlist
        beta/ : E123,W234
        # another comment and space separated error list
        gamma/*.py: E123, W234, E111
        # oh no, multiple files!
        alpha/,delta/,*.py: E,W
        # multiple on the same line
        first_file.py: W9 second_file.py: F4,F9 third_file.py : F4
        sub_dir/* :F4
        fourth_file.py, fifth_file.py: W9, F9
        sixth_file.py:
            E  # comment
            W  # comment
            F malformed_file.py: E911
        # oh look the second set has two paths
        seventh_file.py: E123 eighth_file.py,       ninth_file.py : W

        not_a_file : ":"  # literal colon for whatever reason
        key_with_no_value :
        hanging,  # comment
        comma,  # comment

        delimited,  # comment

        keys,  # comment
        :

        value,  # comment
        with,  # comment


        hanging,  # comment
        comma,  # comment
        space separated key: space separated value
    '''
    expected = [
        ("test.py", "E123"),
        ("beta/", "E123, W234"),
        ("gamma/*.py", "E123, W234, E111"),
        ("alpha/, delta/, *.py", "E, W"),
        ("first_file.py", "W9"),
        ("second_file.py", "F4, F9"),
        ("third_file.py", "F4"),
        ("sub_dir/*", "F4"),
        ("fourth_file.py, fifth_file.py", "W9, F9"),
        ("sixth_file.py", "E, W, F"),
        ("malformed_file.py", "E911"),
        ("seventh_file.py", "E123"),
        ("eighth_file.py, ninth_file.py", "W"),
        ("not_a_file", ":"),
        ("key_with_no_value", ""),
        ("hanging, comma, delimited, keys", "value, with, hanging, comma"),
        ("space separated key", "space separated value")
    ]

    assert utils.parse_key_value_pairs(string) == expected

I was planning on using the tokenize library to implement this especially since it's already imported in the utils module. Any thoughts?

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 7, 2018, 05:25

Commented on src/flake8/main/options.py line 150

You're right about the declarative style and it is hard to name things. Perhaps just mapping or value_map.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 7, 2018, 05:25

So, I'd like to keep the implementation small for now. We can expand if necessary but I'd rather keep it simple.

For example, I think it's okay if we parse what you suggested here, but I'm not fond of having multiple values on the key part of the mapping. So I would rather we not try to further parse a key like alpha/, delta/, *.py I can see how that might be useful, but I'd also rather find the demand for that before trying to implement it.

I suspect we will need to allow spaces in the filename though because I'm sure people name scripts that they execute with spaces (although that would never work for importable modules, of course).

I'm also a little hesitant about using tokenize because it has some very weird behaviour across versions of Python, but I'm open to seeing how you accomplish this and it does seem like it would be the right tool for the job.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Nov 17, 2018, 05:44

@demosdemon I'm going to make it clear that this blocks a new release for us because there are issues with how we're parsing the ignores out. When you have the time to work on this, we'd greatly appreciate your contribution. If you find you no longer have time or interest, let us know and one of us can try to take it over and drive it to completion.

Thanks in advance

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Jan 10, 2019, 14:44

I took a stab at this in !281 -- let me know what you think

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jan 17, 2019, 05:53

Fixed by !281

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jan 17, 2019, 05:53

closed

@asottile asottile closed this as completed Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gitlab merge request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@asottile