Skip to content

Conversation

@ianna
Copy link
Member

@ianna ianna commented Nov 27, 2023

It seems to me that creating global constants in the tests is unnecessary. PyCall checks if a Python package has been imported and reuses it.

@ianna ianna marked this pull request as draft November 27, 2023 14:44
@ianna ianna force-pushed the ianna/remove-global-constants branch from 9e1ef67 to 66d193a Compare November 27, 2023 16:21
@codecov
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bed29bc) 71.32% compared to head (49e3b7b) 71.32%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #46   +/-   ##
=======================================
  Coverage   71.32%   71.32%           
=======================================
  Files           3        3           
  Lines        1618     1618           
=======================================
  Hits         1154     1154           
  Misses        464      464           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Moelf
Copy link
Member

Moelf commented Nov 27, 2023

Instead of installing test dependency on the fly, it's cleaner to have a test Project.toml, for example: https://github.com/JuliaHEP/UnROOT.jl/blob/main/test/Project.toml . They will be installed in addition to the package you're testing

@ianna
Copy link
Member Author

ianna commented Nov 28, 2023

Instead of installing test dependency on the fly, it's cleaner to have a test Project.toml, for example: https://github.com/JuliaHEP/UnROOT.jl/blob/main/test/Project.toml . They will be installed in addition to the package you're testing

I think, I'd like to have PyCall as an optional dependency of the AwkwardArray module and only for the AwkwardPyCall module.

@ianna ianna marked this pull request as ready for review November 28, 2023 09:29
@ianna ianna changed the title Ianna/remove global constants fix: remove global constants Nov 28, 2023
@ianna ianna requested a review from Moelf November 28, 2023 09:35
@Moelf
Copy link
Member

Moelf commented Nov 28, 2023

That's fine, you're doing the Pkg extension mechanism right? The test project.toml is the correct way to test that setup, basically that would simulate when user have AK.jk and PyCall.jl installed

@Moelf
Copy link
Member

Moelf commented Nov 28, 2023

we can merge this and I can make a PR later to move to Pkg extension system

@ianna ianna merged commit 9f125f1 into main Nov 28, 2023
@ianna ianna deleted the ianna/remove-global-constants branch November 28, 2023 12:20
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