Skip to content

Add mypy_path config option (#2323) #2329

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

Merged
merged 7 commits into from
Oct 28, 2016
Merged

Add mypy_path config option (#2323) #2329

merged 7 commits into from
Oct 28, 2016

Conversation

ffigiel
Copy link
Contributor

@ffigiel ffigiel commented Oct 25, 2016

I tried to write a simple test for the new build_lib_path function, but pytest wouldn't pick it up. Help?

@codecov-io
Copy link

codecov-io commented Oct 25, 2016

Current coverage is 83.48% (diff: 47.36%)

Merging #2329 into master will decrease coverage by <.01%

@@             master      #2329   diff @@
==========================================
  Files            72         72          
  Lines         19012      19018     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       3931       3932     +1   
==========================================
+ Hits          15873      15877     +4   
- Misses         2542       2543     +1   
- Partials        597        598     +1   

Powered by Codecov. Last update d0ea126...fe82138

@ddfisher
Copy link
Collaborator

I don't see the test you mentioned in the PR, so it's hard for me to say what might've gone wrong. If you add it I can take a look.

Copy link
Collaborator

@ddfisher ddfisher left a comment

Choose a reason for hiding this comment

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

Instead of introducing a new stubs_dir concept, I'd much rather see this setting be precisely equivalent to (and superseeded by, if present) MYPYPATH.

@ffigiel
Copy link
Contributor Author

ffigiel commented Oct 26, 2016

Updated - is this ok? As for the test, I figured it will make more sense to add a test case to test-data/unit dir, so I'll try that.

@@ -451,6 +451,7 @@ def get_init_file(dir: str) -> Optional[str]:
'python_version': lambda s: tuple(map(int, s.split('.'))),
'strict_optional_whitelist': lambda s: s.split(),
'custom_typing_module': str,
'mypy_path': lambda s: [p.strip() for p in s.split(',')],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you split by os.pathsep here instead of commas? That way it mirrors MYPYPATH's behavior and is a little more idiomatic.

Also, it's a bit subtle, but I think you shouldn't strip the paths here -- that's not what happens with other paths specifiers (like $PATH) and it makes it impossible to specify a path with a space at the end (though I'd hope no one actually wants to do that).

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 think it's better to stick to a common convention of handling lists in config files. Parsing that setting this way would probably cause confusion.

Copy link
Member

Choose a reason for hiding this comment

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

But the env variable that this is supposed to track uses os.pathsep (':'). So using commas here would also probably cause confusion. Maybe we can compromise and split on spaces, commas and colons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. At that point, this function could have a test too. How can I add a small unit test? All I can find is big test suites or use-case scenarios, and that's too much to test a pure function 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Splitting on commas and colons seems reasonable, but we shouldn't split on spaces: it's not uncommon to have spaces in directory names. It'd be nice to mention allowable path separators in the documentation for this config option.

@gvanrossum do we currently have tests for the config file somewhere?

Copy link
Collaborator

@ddfisher ddfisher left a comment

Choose a reason for hiding this comment

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

This is looking good! Just one more small change here. After that and the unit test, this should be ready to merge!

@gvanrossum
Copy link
Member

gvanrossum commented Oct 27, 2016 via email

@ffigiel ffigiel changed the title Add stubs_dir config option (#2323) Add mypy_path config option (#2323) Oct 28, 2016
@gvanrossum
Copy link
Member

If you rebase now the tests should pass.

@ffigiel
Copy link
Contributor Author

ffigiel commented Oct 28, 2016

I'm up to date

@gvanrossum
Copy link
Member

OK, I'm going to look into why the tests fail then. Because that failure seems due to a missed typeshed sync to me (you didn't touch reports.py, there was a recent change to reports.py that depended on some typeshed changes).

@ddfisher
Copy link
Collaborator

Yeah, it looks like typeshed is out of date. You can fix that with git checkout master -- typeshed && git commit -a.

@ddfisher
Copy link
Collaborator

This looks good! We should probably change os.pathsep back to colon, actually -- otherwise mypy.ini files won't work cross-platform properly. :/ Sorry for flip-flopping about that!

@ffigiel
Copy link
Contributor Author

ffigiel commented Oct 28, 2016

Ok, will do - thanks for noticing that issue @ddfisher!

@ffigiel
Copy link
Contributor Author

ffigiel commented Oct 28, 2016

I did git checkout upstream/master typeshed and the submodule commit changed, so it was probably an error on my side after all. Sorry for that! 🙇

@ddfisher
Copy link
Collaborator

Thanks!

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.

4 participants