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

Separate pyproject.toml location from root argument #244

Merged
merged 4 commits into from
Dec 27, 2022

Conversation

mkniewallner
Copy link
Collaborator

@mkniewallner mkniewallner commented Dec 26, 2022

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Description of changes

deptry allows passing the root directory as a positional argument, which gives some flexibility to projects that use different kind of structures.

This is for instance useful for projects that use an src directory structure, like Poetry or Hatch.

Unfortunately, the root directory passed as a command line means that the configuration file will be read from the directory, and not from the directory deptry is launched from, diverging, in my opinion, to what most other tools in the ecosystem do.

This PR is an attempt to fix that by always trying to load pyproject.toml from the location deptry is run from, both for reading [tool.deptry] configuration, or extracting the dependencies if a project uses PEP 621 or Poetry.

This means that when running:

$ deptry src

deptry will now search for pyproject.toml in the location it is run from, whereas currently, doing so will make deptry look for a pyproject.toml in src directory.

The downside is that this could break some projects that did explicitly want to find pyproject.toml in a root directory other than ., but I have a second PR (#245) building upon this one that exposes the --config argument, making it possible to explicitly pass the location of pyproject.toml, giving both flexibility for passing a different root directory and allowing to continue supporting this use case, if it exists.

@codecov-commenter
Copy link

Codecov Report

Merging #244 (cfe17dc) into main (a2866fe) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main    #244     +/-   ##
=======================================
- Coverage   95.7%   95.6%   -0.1%     
=======================================
  Files         32      32             
  Lines        915     906      -9     
  Branches     203     200      -3     
=======================================
- Hits         876     867      -9     
  Misses        26      26             
  Partials      13      13             
Impacted Files Coverage Δ
deptry/cli.py 93.4% <100.0%> (-0.2%) ⬇️
deptry/core.py 97.6% <100.0%> (ø)
deptry/utils.py 86.6% <100.0%> (-4.7%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mkniewallner
Copy link
Collaborator Author

Keeping it as a draft for now, since this might be controversial, and I would like to have your opinion on the changes first.

Copy link
Owner

@fpgmaas fpgmaas left a comment

Choose a reason for hiding this comment

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

I think this is a very welcome change, thanks! It did not cross my mind, but it seems to be what tools like isort and black do as well, so probably also the behaviour users would expect.

As you mentioned, this does mean that we could potentially introduce breaking changes in other projects using deptry. I think it would be good to not create a release after merging this PR, and then - since the project is also approaching reasonable maturity - to create our first major release after merging both this PR and #245.

Do you agree?

@mkniewallner
Copy link
Collaborator Author

As you mentioned, this does mean that we could potentially introduce breaking changes in other projects using deptry. I think it would be good to not create a release after merging this PR, and then - since the project is also approaching reasonable maturity - to create our first major release after merging both this PR and #245.

Do you agree?

Semantic versioning explicitly authorises introducing breaking changes in 0.y.z.

Personally, for now, I like that we are able to update the API of the CLI or the library's behaviour without having to give too much thoughts about it, even if it is still a good thing to not break possible current usages of it (like this PR + #245 do, by offering alternatives), and of course, communicate about the changes we are making in the changelog.

If we want to respect semantic version, going into 1.0.0 would be a major shift in how we handle changes in the project, and I can see a few things that could be reworked a bit before that, especially considering that deptry is not yet used in major projects, so we might see multiple feature requests over time that could considerably change the current APIs or behaviour of the library.

There are IMO reasons why major libraries like black took quite some time before going into "stable", or why mypy is still on a 0.y versioning: it takes time to stabilise APIs based on users expectations, what possible changes we think of introducing as developers, and it's also a point of no return.

In any case, I agree that it would be best to tag a release once #245 is also merged, but based on everything mentioned above, I would personally bump the version to 0.7.0, and mention both changes in the changelog. Of course, you might disagree with me about not going into 1.0.0, but in that case, I'd like that we at least talk about it in a bit more details, as this IMO requires careful planning.

@mkniewallner mkniewallner marked this pull request as ready for review December 27, 2022 10:14
@fpgmaas
Copy link
Owner

fpgmaas commented Dec 27, 2022

Thanks, that makes sense. I was unaware that breaking changes can be introduced in version 0.y.z. Although in hindsight it makes sense :) I agree with your proposal of releasing 0.7.0 instead.

@mkniewallner mkniewallner merged commit efcfc03 into fpgmaas:main Dec 27, 2022
@mkniewallner mkniewallner deleted the feat/explicit-root-discovery branch December 27, 2022 10:33
@fpgmaas fpgmaas mentioned this pull request Dec 27, 2022
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.

3 participants