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

Detect changes in environment-file and update as needed #17

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

kunaltyagi
Copy link
Contributor

No description provided.

@kunaltyagi
Copy link
Contributor Author

Fixes #12 (once complete)

Please let me know if this is the right direction to go towards

@kunaltyagi kunaltyagi marked this pull request as ready for review December 2, 2023 17:07
@kunaltyagi
Copy link
Contributor Author

@OldGrumpyViking PTAL

@OldGrumpyViking
Copy link
Owner

Hi @kunaltyagi, thanks for this update. Please update the history.md and solve the conflict and i will merge it in and make a release.

@kunaltyagi
Copy link
Contributor Author

PTAL

@harelwa
Copy link
Contributor

harelwa commented Jan 8, 2024

Hi @kunaltyagi

Thank you for this PR.

I wanted to ask if this implementation is aligned (in spirit :) with hatch v1.8 improvements on the same topic https://hatch.pypa.io/1.8/blog/2023/12/11/hatch-v180/#faster-environment-usage ?

Thanks,

Harel

@kunaltyagi
Copy link
Contributor Author

I dunno. Do we need to do something special?

I did modify the check function (https://hatch.pypa.io/latest/plugins/environment/reference/#hatch.env.plugin.interface.EnvironmentInterface.dependencies_in_sync), but let me know if we should add something else as well

@harelwa
Copy link
Contributor

harelwa commented Jan 8, 2024

I dunno. Do we need to do something special?

I did modify the check function (https://hatch.pypa.io/latest/plugins/environment/reference/#hatch.env.plugin.interface.EnvironmentInterface.dependencies_in_sync), but let me know if we should add something else as well

I would review the hatch core implementation to and make sure the functionality is actually achieved and tested, as i'm not sure i understand how you avoid some sort of caching, checksum etc.

https://github.com/pypa/hatch/blob/7b1ef02af717101d006d9801bca6f246ecb46892/backend/src/hatchling/dep/core.py#L123

@kunaltyagi
Copy link
Contributor Author

I don't think I need to. There are no suggestions or warnings in the docs, so the usage should be straightforward

Please let me know if you have seen some regression. Otherwise I don't want to spend time on something that the plugin api should achieve automatically

@OldGrumpyViking OldGrumpyViking merged commit 5086e15 into OldGrumpyViking:main Jan 9, 2024
5 checks passed
@harelwa
Copy link
Contributor

harelwa commented Jan 9, 2024

@OldGrumpyViking

I see you have merged this to main.

Please do not publish this, it's breaking the very basic usage of this plugin.

This PR needs more tests to literally prove it's doing what it's suppose to be doing (i'm not sure it does, it's not really tested)

It also needs to prove it's not degrading "built in" sync when dependencies in hatch.toml env def are changed ( another required test ).

Unfortunately i do not have time to further dive into it.

Thanks,
Harel

@OldGrumpyViking
Copy link
Owner

@kunaltyagi can you address the issues mentioned by @harelwa.

@kunaltyagi
Copy link
Contributor Author

Created pr to do that

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