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

explicitly check for first-party #1071

Merged
merged 2 commits into from
May 21, 2020
Merged

Conversation

honnix
Copy link
Contributor

@honnix honnix commented Dec 19, 2019

modules under pwd could be identified as first-party instead of
default configuration

@timothycrosley wdyt of this?

@honnix
Copy link
Contributor Author

honnix commented Dec 19, 2019

The Poetry configuration is invalid:
  - Additional properties are not allowed ('website' was unexpected)

Incompatible poetry?

Update:

Tried a few things and couldn't get it to work.

@honnix
Copy link
Contributor Author

honnix commented Dec 19, 2019

This is a breaking change though because it would sort/separate imports differently than today.

To address #704 without installing all third party dependencies into virtual, one could configure default to be THIRDPARTY.

@timothycrosley
Copy link
Member

modules under pwd could be identified as first-party instead of default configuration

I think this (pwd or specified src directory) is the correct approach and would like to get this in for the 5.0.0 release, thanks for the pull-request!

@honnix
Copy link
Contributor Author

honnix commented Jan 5, 2020

Great, thanks! Should I try again to get CI fixed? That feels better to be a separated PR though.

@honnix
Copy link
Contributor Author

honnix commented Jan 23, 2020

@timothycrosley ping

@AdmiralNemo
Copy link

I came across this PR while researching how isort determines whether a package is "first party" vs "third party."

Personally, I don't think this is a good way to determine the difference. I have a lot of per-project virtual environments, which I keep inside the project directory in a .venv subdirectory. This change would end up marking everything as first party for these projects, since the virtual environment's "site-packages" directory path contains the current working directory path.

If the "first party" path could be made configurable (i.e. only things under src/), that would be fine, but please don't assume that everything in ${PWD} is first party.

@honnix
Copy link
Contributor Author

honnix commented Jan 25, 2020

I think this happens after checking for 3rd party.

@honnix
Copy link
Contributor Author

honnix commented Jan 25, 2020

@AdmiralNemo
Copy link

Ah, interesting. There must be something else going on that I do not understand, because the only way I have ever managed to get isort to classify anything as "third-party" is to put default_section=THIRDPARTY, and then explicitly list my own packages in known_first_party. I was afraid that this change would prevent that, but it seems like there is some other problem I need to address. 🤷‍♂️

@timothycrosley
Copy link
Member

#1147 specifies the desired behaviour change along these lines

modules under pwd could be identified as first-party instead of
default configuration
@codecov-commenter
Copy link

Codecov Report

Merging #1071 into develop will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1071      +/-   ##
===========================================
- Coverage    99.95%   99.90%   -0.05%     
===========================================
  Files           32       32              
  Lines         2028     2030       +2     
===========================================
+ Hits          2027     2028       +1     
- Misses           1        2       +1     

@timothycrosley timothycrosley merged commit 71da3e2 into PyCQA:develop May 21, 2020
@honnix honnix deleted the first-party branch August 19, 2020 07:49
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

4 participants