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

Don't change poetry.lock during poetry update --dry-run #3767

Closed
wants to merge 1 commit into from
Closed

Don't change poetry.lock during poetry update --dry-run #3767

wants to merge 1 commit into from

Conversation

martinmo
Copy link

@martinmo martinmo commented Mar 10, 2021

Pull Request Check List

Resolves: #3766

  • Added tests for changed code.
  • Updated documentation for changed code.

AFAIK, there is no relevant documentation to be changed, am I right?

@martinmo
Copy link
Author

Linting fails on a file that is not touched in this PR (docs/docs/pyproject.md).

@abn
Copy link
Member

abn commented Mar 21, 2021

@martinmo can you rebase the branch please? Should fix the linting issues.

Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

Needs a rebase to fix checks.

@martinmo
Copy link
Author

Needs a rebase to fix checks.

Done ✅

@martinmo martinmo requested a review from abn March 23, 2021 13:59
@martinmo
Copy link
Author

I have rebased this to check if it works against the current code in master. This PR probably also fixes #3739 (lock --dry-run) and #3666 (remove --dry-run), or basically every command that has a --dry-run flag wrt to the lockfile and doesn't set the force parameter explicitely when calling _write_lock_file(…).

@finswimmer
Copy link
Member

Hello @martinmo,

could you please rebase once more to force the tests running again?

Thanks!

fin swimmer

@martinmo
Copy link
Author

Hello @martinmo,

could you please rebase once more to force the tests running again?

Thanks!

fin swimmer

Done ✅

@getup8
Copy link

getup8 commented Jan 4, 2022

Hi @martinmo just checking on this and seeing if you had the time to push this through; it's a pretty annoying part of poetry! Thanks :)

@martinmo
Copy link
Author

martinmo commented Jan 5, 2022

Hi @martinmo just checking on this and seeing if you had the time to push this through; it's a pretty annoying part of poetry! Thanks :)

Not yet, but I rebased and force pushed it again to see what the CI checks say.

@martinmo
Copy link
Author

martinmo commented Jan 5, 2022

Hi @abn could you please have another look at this PR? I rebased it and adjusted it to conform to the new coding guidelines. Thank you!

@martinmo
Copy link
Author

martinmo commented Jan 6, 2022

@finswimmer the review for this PR is somehow stuck. I saw that you have been active on other PRs in the last days, could you please have a look at this PR? It has a test for the changed behavior and all CI checks have passed. Thanks!

@dimitrismistriotis
Copy link

Me + team affected from this, would be great if it could be progressed.

@zEdS15B3GCwq
Copy link

I'm on 1.2.0b1 (current latest) and this issue persist. Please proceed with the PR. Much appreciated.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

At first, thanks for your contribution and your patience.

I think the fix may not cover all cases. I'd really appreciate it if you could write tests for the other commands (see below) and check if my assumption is correct.

@@ -355,7 +355,7 @@ def _do_install(self, local_repo: Repository) -> int:
# Execute operations
return self._execute(ops)

def _write_lock_file(self, repo: Repository, force: bool = True) -> None:
def _write_lock_file(self, repo: Repository, force: bool = False) -> None:
if force or (self._update and self._write_lock):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if force or (self._update and self._write_lock):
if (force or self._update) and self._write_lock:

I think that line is also wrong since self._write_lock is only used for --dry-run, force should not overwrite this. From inspecting the code, I assume that fix is necessary for poetry add --lock --dry-run. I really think we need more coverage, here.

)


def test_dry_run_update(
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add similar tests (or extend existing tests) for the following commands (if not yet existing):

  • add --dry-run
  • add --lock --dry-run
  • remove --dry-run

Further, it seems that tests for add check that the pyproject.toml is not changed for dry-run. Can you please add this check here, too?

Copy link
Member

Choose a reason for hiding this comment

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

And probably, you should rebase first before adding tests. 🙏

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

poetry update --dry-run modifies poetry.lock
8 participants