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

Fix for #832 #1052

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix for #832 #1052

wants to merge 2 commits into from

Conversation

Edwin18
Copy link

@Edwin18 Edwin18 commented Dec 22, 2023

frontend.py: Fix function argument

Previosly in function was passed raw value self.ignore_dirs, not a list as expected ignore_dirs.

@akx
Copy link
Member

akx commented Dec 22, 2023

Thanks! It would be nice to see a test for this, since the tests I wrote for #832 clearly didn't catch this typo 😅

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.99%. Comparing base (e0d1018) to head (f8f15ef).

❗ Current head f8f15ef differs from pull request most recent head a6c692b. Consider uploading reports for the commit a6c692b to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1052   +/-   ##
=======================================
  Coverage   90.99%   90.99%           
=======================================
  Files          26       26           
  Lines        4444     4444           
=======================================
  Hits         4044     4044           
  Misses        400      400           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Edwin18
Copy link
Author

Edwin18 commented Dec 22, 2023

Okay i will check what i can do 😉

@Edwin18
Copy link
Author

Edwin18 commented Dec 22, 2023

Very simple minor fix 711676b

To make test work properly just add another value to our ignore list and it imidiately fails.
Multiple args works fine:
--ignore-dirs '_*' --ignore-dirs '*ignored*'
but not multiple values:
--ignore-dirs '*ignored* _*'

Before, function argument fix:

=============test session starts=============
platform linux -- Python 3.10.11, pytest-7.4.3, pluggy-1.3.0 -- /home/mrgreen/code/babel/venv/bin/python3
cachedir: .pytest_cache
rootdir: /home/mrgreen/code/babel
configfile: setup.cfg
plugins: cov-4.1.0
collected 60 items / 58 deselected / 2 selected                                                                                                                                                                                                                                                         

tests/messages/test_frontend.py::test_extract_ignore_dirs[False] 
ignore_dirs = ['*ignored*', '.*'] 
FAILED
tests/messages/test_frontend.py::test_extract_ignore_dirs[True] 
ignore_dirs = ['*ignored*', '.*', '_*'] 
FAILED

=============FAILURES =============
______________________ test_extract_ignore_dirs[False] ______________________

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7febbf363280>, capsys = <_pytest.capture.CaptureFixture object at 0x7febbf363520>, tmp_path = PosixPath('/tmp/pytest-of-mrgreen/pytest-33/test_extract_ignore_dirs_False0'), with_underscore_ignore = False

    @pytest.mark.parametrize("with_underscore_ignore", (False, True))
    def test_extract_ignore_dirs(monkeypatch, capsys, tmp_path, with_underscore_ignore):
        pot_file = tmp_path / 'temp.pot'
        monkeypatch.chdir(project_dir)
        cmd = f"extract . -o '{pot_file}' --ignore-dirs '*ignored* .*' "
        if with_underscore_ignore:
            # This also tests that multiple arguments are supported.
            cmd += "--ignore-dirs '_*'"
        cmdinst = configure_cli_command(cmd)
        assert isinstance(cmdinst, ExtractMessages)
        assert cmdinst.directory_filter
        cmdinst.run()
        pot_content = pot_file.read_text()
    
        # The `ignored` directory is now actually ignored:
>       assert 'this_wont_normally_be_here' not in pot_content
E       assert 'this_wont_normally_be_here' not in '# Translati...tr[1] ""\n\n'
E         'this_wont_normally_be_here' is contained here:
E           # Translations template for PROJECT.
E           # Copyright (C) 2023 ORGANIZATION
E           # This file is distributed under the same license as the PROJECT project.
E           # FIRST AUTHOR <EMAIL@ADDRESS>, 2023.
E           #
E           #, fuzzy...
E         
E         ...Full output truncated (33 lines hidden), use '-vv' to show

/home/mrgreen/code/babel/tests/messages/test_frontend.py:1588: AssertionError
______________________ test_extract_ignore_dirs[True] ______________________

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7febbeebbcd0>, capsys = <_pytest.capture.CaptureFixture object at 0x7febbeebb550>, tmp_path = PosixPath('/tmp/pytest-of-mrgreen/pytest-33/test_extract_ignore_dirs_True_0'), with_underscore_ignore = True

    @pytest.mark.parametrize("with_underscore_ignore", (False, True))
    def test_extract_ignore_dirs(monkeypatch, capsys, tmp_path, with_underscore_ignore):
        pot_file = tmp_path / 'temp.pot'
        monkeypatch.chdir(project_dir)
        cmd = f"extract . -o '{pot_file}' --ignore-dirs '*ignored* .*' "
        if with_underscore_ignore:
            # This also tests that multiple arguments are supported.
            cmd += "--ignore-dirs '_*'"
        cmdinst = configure_cli_command(cmd)
        assert isinstance(cmdinst, ExtractMessages)
        assert cmdinst.directory_filter
        cmdinst.run()
        pot_content = pot_file.read_text()
    
        # The `ignored` directory is now actually ignored:
>       assert 'this_wont_normally_be_here' not in pot_content
E       assert 'this_wont_normally_be_here' not in '# Translati...tr[1] ""\n\n'
E         'this_wont_normally_be_here' is contained here:
E           # Translations template for PROJECT.
E           # Copyright (C) 2023 ORGANIZATION
E           # This file is distributed under the same license as the PROJECT project.
E           # FIRST AUTHOR <EMAIL@ADDRESS>, 2023.
E           #
E           #, fuzzy...
E         
E         ...Full output truncated (29 lines hidden), use '-vv' to show

/home/mrgreen/code/babel/tests/messages/test_frontend.py:1588: AssertionError

After, function argument fix:

============= test session starts =============
platform linux -- Python 3.10.11, pytest-7.4.3, pluggy-1.3.0 -- /home/mrgreen/code/babel/venv/bin/python3
cachedir: .pytest_cache
rootdir: /home/mrgreen/code/babel
configfile: setup.cfg
plugins: cov-4.1.0
collected 60 items / 58 deselected / 2 selected                                                                                                                                                                                                                                                         

tests/messages/test_frontend.py::test_extract_ignore_dirs[False] 
ignore_dirs = ['*ignored*', '.*'] 
PASSED
tests/messages/test_frontend.py::test_extract_ignore_dirs[True] 
ignore_dirs = ['*ignored*', '.*', '_*'] 
PASSED

@Edwin18
Copy link
Author

Edwin18 commented Dec 22, 2023

And i have a question/suggestion about this one.

def default_directory_filter(dirpath: str | os.PathLike[str]) -> bool:
subdir = os.path.basename(dirpath)
# Legacy default behavior: ignore dot and underscore directories
return not (subdir.startswith('.') or subdir.startswith('_'))

if directory_filter is None:
directory_filter = default_directory_filter

So in current realization we can have or default or user ignore list, maybe we should extend default one with user if it provided?

def _make_directory_filter(ignore_patterns):
    """
    Build a directory_filter function based on a list of ignore patterns.
    """

    if not ignore_patterns:
        ignore_patterns = []

    def cli_directory_filter(dirname):
        basename = os.path.basename(dirname)
        return not any(
            fnmatch.fnmatch(basename, ignore_pattern)
            for ignore_pattern
            in [*ignore_patterns, '.*', '_*']
        )

    return cli_directory_filter

Previosly in function was passed raw value 'self.ignore_dirs', not a list as expected 'ignore_dirs'.

Fixes for python-babel#832
Adds multiple value for proper test.

Fixes for python-babel#832
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.

None yet

3 participants