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

allow registering from a dirty repo #87

Merged
merged 4 commits into from
May 30, 2024

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented May 28, 2024

I often want to register a package dir that is dirty – typically, when I have some example notebooks in the dir and playing with them. In the simplest case, it requires manually stashing & unstashing changes. But even this doesn't always work when notebooks are currently open in Pluto because it updates some fields such as version continuously.

Would be nice to add a flag like allow_dirty (open to name suggestions) that would just register the current commit and ignore the git repo being dirty.

Will add a test if this addition is welcome!

@GunnarFarneback
Copy link
Owner

The main concern with allowing the package to be dirty is if Project.toml is dirty and there is a mismatch between the data read from file and the data recorded in the tree hash. Possibly that (i.e. Project.toml being dirty) should still not be allowed.

This patch conflates dirtiness of package and registry, which are quite different cases. For the registry I think allowing it to be dirty if and only if commit is false is still the right thing to do. Consequently the argument name should be more clear that it refers to the package.

The new argument needs documentation in the docstring.

@aplavin
Copy link
Contributor Author

aplavin commented May 28, 2024

Totally agree that such a behavior has a potential for mismatches, and shouldn't be the default. As an option though, ignoring dirty files is definitely useful.

Renamed the argument, made it only affect the package dir, and added to the docs + tests.

@GunnarFarneback
Copy link
Owner

I'll merge this as is and will follow up with some additions before I register a new version. Thanks for the contribution.

@GunnarFarneback GunnarFarneback merged commit 7d444a1 into GunnarFarneback:master May 30, 2024
8 of 9 checks passed
@aplavin
Copy link
Contributor Author

aplavin commented May 30, 2024

Thanks!
LocalRegistry is generally very nice to use, this is the only significant inconvenience I noticed in extensive use over multiple years :)

@aplavin
Copy link
Contributor Author

aplavin commented Jul 8, 2024

Still cannot use this argument... Could you please register the new version?

@GunnarFarneback
Copy link
Owner

Sorry, have been sidetracked with other things. I'll get back to it.

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.

2 participants