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

Add --ignore flag to ignore files/directories #14

Merged
merged 12 commits into from
May 16, 2021
Merged

Add --ignore flag to ignore files/directories #14

merged 12 commits into from
May 16, 2021

Conversation

takos22
Copy link
Contributor

@takos22 takos22 commented Apr 18, 2021

Add --ignore flag to ignore files/directories
Alias -I, can be used multiple times

Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good! For this to be merged, I'd like to see some tests for the new feature, and maybe a one-line note in the changelog.

findimports.py Outdated Show resolved Hide resolved
findimports.py Outdated Show resolved Hide resolved
findimports.py Outdated Show resolved Hide resolved
findimports.py Show resolved Hide resolved
@takos22 takos22 requested a review from mgedmin April 20, 2021 19:16
@takos22
Copy link
Contributor Author

takos22 commented Apr 20, 2021

Done the changes

@mgedmin
Copy link
Owner

mgedmin commented Apr 21, 2021

Excellent! Now the only missing thing is unit tests for the new code:

Name             Stmts   Miss  Cover   Missing
----------------------------------------------
findimports.py     525      2    99%   434, 436
----------------------------------------------
TOTAL              525      2    99%
Coverage failure: total of 100 is less than fail-under=100

It would be easier to write the tests after some refactoring of the code. I would extract the filtering logic into a separate method:

    def filterIgnoes(self, dirs, files, ignores):
        for ignore in ignores:
            if ignore in dirs:
                dirs.remove(ignore)
            if ignore in files:
                files.remove(ignore)

call it inside parsePathname(), and then write a test like

    def test_filterIgnores(self):
        dirs = ['venv', 'submodule']
        files = ['code.py', 'README.txt']
        mg = findImports.ModuleGraph()
        mg.filterIgnores(dirs, files, ignores=['venv', 'README.txt'])
        self.assertEqual(dirs, ['submodule'])
        self.assertEqual(files, ['code.py'])

in tests.py, next to the parsePathname test.

@takos22
Copy link
Contributor Author

takos22 commented May 2, 2021

I was busy lately but I will try to make the changes tomorrow.

@mgedmin
Copy link
Owner

mgedmin commented May 3, 2021

There's no rush! ;)

@takos22
Copy link
Contributor Author

takos22 commented May 16, 2021

Separated the logic in ModuleGraph.filterIgnores, added tests, updated the docstring in findimports.py, bumped the version to 0.2.1 and added a short description in CHANGES.rst. Could you review again?

Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Ignore a file or directory; this option can be used
multiple times. Default: ['venv']

FindImports requires Python 3.6 or later.
Copy link
Owner

Choose a reason for hiding this comment

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

Oh wow, and I'd totally forgotten that there was a usage message in the docstring! Thank you for noticing and updating it!

(This code is old, these days I'd rely on argparse-generated help messages, and maybe put a copy in the README for new user convenience, and then usually forget to update it when making changes.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm making modules too so I know the feeling when I forget to update docs 😅

@mgedmin mgedmin merged commit 2f4cd82 into mgedmin:master May 16, 2021
@mgedmin
Copy link
Owner

mgedmin commented May 16, 2021

I've pushed a new release to PyPI.

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

Successfully merging this pull request may close these issues.

2 participants