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

Begin implementing null backend #29

Merged
merged 5 commits into from
Mar 10, 2022
Merged

Conversation

kescobo
Copy link
Contributor

@kescobo kescobo commented Mar 7, 2022

If you'd rather deal with this yourself, fine with me if you want to just close this. Closes #28

I'm not totally sure how to test this, since it would require getting the workflow runner to install the appropriate packages separately to make the other tests pass. But if you're ok with it, I think adding a disclaimer that this is untested would be ok 🤷

@cjdoris
Copy link
Collaborator

cjdoris commented Mar 8, 2022

Thanks, be my guest :)

Off the top of my head, these API functions will need to change when backend is Null:

  • envdir - throw an error, and document it can throw?
  • withenv - already covered by activate!, but could also return f()
  • which - just return Sys.which(progname)
  • resolve - immediately return true
  • gc - do nothing
  • status - needs some logic to avoid checking+printing the currently installed versions

src/backend.jl Show resolved Hide resolved
@kescobo
Copy link
Contributor Author

kescobo commented Mar 8, 2022

  • envdir - throw an error, and document it can throw?
  • withenv - already covered by activate!, but could also return f()
    • The stuff before the try block isn't all that expensive - if it were me, I'd leave it to fall back to activate!(). But I put it in for now.
  • which - just return Sys.which(progname)
  • resolve - immediately return true
    • Other parts of this function return nothing, I just added the check to the first conditional block.
  • gc - do nothing
  • status - needs some logic to avoid checking+printing the currently installed versions

@kescobo kescobo closed this Mar 8, 2022
@kescobo kescobo reopened this Mar 8, 2022
@kescobo
Copy link
Contributor Author

kescobo commented Mar 8, 2022

Oops - didn't mean to close that 😆

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #29 (192416c) into main (88248ae) will decrease coverage by 2.53%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
- Coverage   93.32%   90.78%   -2.54%     
==========================================
  Files           7        7              
  Lines         584      597      +13     
==========================================
- Hits          545      542       -3     
- Misses         39       55      +16     
Impacted Files Coverage Δ
src/backend.jl 41.66% <50.00%> (-41.67%) ⬇️
src/deps.jl 92.98% <100.00%> (+0.12%) ⬆️
src/env.jl 98.14% <100.00%> (+0.14%) ⬆️
src/resolve.jl 91.06% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88248ae...192416c. Read the comment docs.

@cjdoris
Copy link
Collaborator

cjdoris commented Mar 8, 2022

Looks good so far!

  • Sure, I'm happy to put withenv back. The less special-casing the better.
  • You're right about resolve, I was confusing it with something else.
  • You added a message to status but there also needs some special casing to treat Null as always being resolved, but not actually displaying any installed versions. If you don't want to do that I'm happy to after the PR is merged.

It would be nice to test this, at least a bit. Maybe the tests could look at JULIA_CONDAPKG_BACKEND and modify the tests appropriately if it is Null?

src/resolve.jl Show resolved Hide resolved
@cjdoris
Copy link
Collaborator

cjdoris commented Mar 8, 2022

On status, specifically it needs to set docurpkgs = resolved && backend() != :Null then replace resolved with docurpkgs in most places.

@kescobo
Copy link
Contributor Author

kescobo commented Mar 10, 2022

On status, specifically it needs to set docurpkgs = resolved && backend() != :Null then replace resolved with docurpkgs in most places.

Here, I just have it return nothing up at the top after printing the message (that's what I originally intended, and just forgot 🤦 ). Is that OK?

@kescobo
Copy link
Contributor Author

kescobo commented Mar 10, 2022

It would be nice to test this, at least a bit. Maybe the tests could look at JULIA_CONDAPKG_BACKEND and modify the tests appropriately if it is Null?

Working on this now

@kescobo
Copy link
Contributor Author

kescobo commented Mar 10, 2022

Not sure if there's a more elegant way to do this, maybe using ReTest.jl or something. But this works.

Can test locally by doing:

julia> using Pkg

julia> withenv("JULIA_CONDAPKG_BACKEND"=> "Null") do
           Pkg.test()
       end

@cjdoris
Copy link
Collaborator

cjdoris commented Mar 10, 2022

Looks great, all your comments are fine.

Boldly merging even though there are 2 tests still running.

Thanks very much for your contribution.

@cjdoris cjdoris merged commit 139494f into JuliaPy:main Mar 10, 2022
@kescobo
Copy link
Contributor Author

kescobo commented Mar 11, 2022

You're welcome! Thanks for quick review and help :-)

@kescobo kescobo deleted the null-backend branch March 17, 2022 17:02
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.

To do: add a Null backend
2 participants