possibility to collapse only external deps to packages #26
Conversation
findimports.py
Outdated
| @@ -97,7 +99,7 @@ | |||
| from operator import attrgetter | |||
|
|
|||
|
|
|||
| __version__ = '2.3.1.dev0' | |||
| __version__ = '2.3.0' | |||
There was a problem hiding this comment.
Please do not touch __version__ . As part of the release process I remove the .dev0 suffix and check for the need of semver-ish version number increases by looking at the changelog (in CHANGES.rst).
I used to preemptively increase the next upcoming version (like '2.3.1.dev0' -> '2.4.0.dev0') when I implemented a new feature, but this way it's easy to accidentally bump the minor version twice, so I mostly stopped.
There was a problem hiding this comment.
That was accidental. I did work in site packages, then copied over. my bad
findimports.py
Outdated
| @@ -1005,6 +1019,8 @@ def main(argv=None): | |||
| args = parser.parse_args(args=argv[1:] if argv else None) | |||
| except SystemExit as e: | |||
| return e.code | |||
| if args.condense_to_packages and args.condense_to_packages_externals: | |||
| raise argparse.ArgumentError('only one of -p and -pE can be provided') | |||
There was a problem hiding this comment.
I think this will print a traceback at the user instead of reporting an error message?
There was a problem hiding this comment.
It does report the message, but there was a bug, message is 2nd arg, which is mandatory.
findimports.py
Outdated
| if externals_only and name in internal_names: | ||
| package_name = name | ||
| else: | ||
| package_name = self.packageOf(name, packagelevel) |
There was a problem hiding this comment.
This is a bit gnarly. I would like a couple of extra functions, maybe isExternal() and maybePackageOf(modname, packagelevel, externals_only) and then hopefully the old loop may suffice?
There was a problem hiding this comment.
Agree it's not ideal from design perspective, but the logic is working correctly there. Maybe leaving some notes for the next time some bigger work is done would suffice?
There was a problem hiding this comment.
That appeared to be more straight forward than I initially thought. done.
findimports.py
Outdated
| g = g.packageGraph(args.packagelevel) | ||
| g = g.packageGraph(args.packagelevel, False) | ||
| elif args.condense_to_packages_externals: | ||
| g = g.packageGraph(args.packagelevel, True) |
There was a problem hiding this comment.
Personal style rule: never pass a True/False using positional argument syntax; always use keyword argument syntax to make it clear what options this thing enables or disables.
|
Fixed the small ones. Do you prefer to make some changes yourself and merge it? As you mentioned some design issues, I think you are in much better position of doing it to your liking. Otherwise, let me know what would be satisfactory for a merge. |
|
Well, unit tests are a requirement. CI currently fails because test coverage fell below 100%. I would like to experiment with the code structure myself, but I'm too lazy to test the changes manually. Having unit tests I could trust would be helpful. |
|
Added one more option, which is to remove prefixes. This one is from Tests should be running now. One more edit was to factor out For more complex dot outputs some class e.g. |
findimports.py
Outdated
| try: | ||
| args = parser.parse_args(args=argv[1:] if argv else None) | ||
| except SystemExit as e: | ||
| return e.code | ||
| if args.condense_to_packages and args.condense_to_packages_externals: | ||
| raise argparse.ArgumentError( | ||
| None, 'only one of -p and -pE can be provided') |
There was a problem hiding this comment.
This is the only statement not covered by the unit tests.
|
Thank you! I will take a closer look a bit later. Please ping me if I seem to forget! |
mgedmin
changed the title
|
Made one more edit. It was still showing: |
|
Added one more feature - ability to specify maximum depth of ast tree. In my case, I sometimes want to see only top level imports as there are some irrelevant ones inside functions. |
|
Do you want a new release? Or do you plan to create more PRs first? |
|
New release would be great, thank you. |
|
Released findimports 2.4.0. |
I tried to implement it with minimal changes.
Closes #25
Would be great to have this one as I most often need to analyse internal dependency structure, while I want to see externals dependencies, but only top levels there as I generally care about dependency as a whole, which part doesn't concern me much as I inevitable depend on the whole of it.