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

(feat): resolving asv env explicitly #35

Merged
merged 21 commits into from
Apr 9, 2024

Conversation

ilan-gold
Copy link
Collaborator

Fixes #31

So obviously this python string business is not ideal but I am not sure what else to do. The python file doesn't copy over with the build after compilation so as far as I can tell we can do it manually or it's PyO3.

@ilan-gold ilan-gold requested a review from flying-sheep April 9, 2024 09:32
Copy link
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

Nice! Just some nitpicks, assuming you want them: There’s two possible ways to make the Python script nicer, and the tests can be a bit more concise.

For further unit tests in this vein, I wonder if we should look into mocking. But that’s for another day.

@ilan-gold
Copy link
Collaborator Author

Thanks so much for my first rust review @flying-sheep 😄

For further unit tests in this vein, I wonder if we should look into mocking. But that’s for another day.

I briefly looked into it, but it seemed like too much of a rabbit hole for right now, agreed.

@ilan-gold ilan-gold requested a review from flying-sheep April 9, 2024 12:04
Copy link
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

I unified the tests a little, looks great!

@flying-sheep flying-sheep merged commit 7c51da9 into main Apr 9, 2024
2 checks passed
@flying-sheep flying-sheep deleted the ig/resolve_env_functionality branch April 9, 2024 13:28
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.

Show comparison for current envs only
2 participants