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

Compatibility with v1.0 #17

Closed
wants to merge 3 commits into from
Closed

Compatibility with v1.0 #17

wants to merge 3 commits into from

Conversation

schillic
Copy link

I see that the tests passed on Julia v1.0 here before. But when I try to build this package with that version, it crashes with the message that pr is a Dict with no field other.

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage: 94.11% and project coverage change: -2.64% ⚠️

Comparison is base (2aec163) 100.00% compared to head (cad271b) 97.36%.

Additional details and impacted files
@@             Coverage Diff             @@
##              main      #17      +/-   ##
===========================================
- Coverage   100.00%   97.36%   -2.64%     
===========================================
  Files            1        1              
  Lines           27       38      +11     
===========================================
+ Hits            27       37      +10     
- Misses           0        1       +1     
Files Changed Coverage Δ
src/PkgVersion.jl 97.36% <94.11%> (-2.64%) ⬇️

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

@KlausC
Copy link
Owner

KlausC commented Aug 28, 2023

Could you, please, add an entry for v1.0 into "CI.yml" in order to maintain the 100% code coverage.

@schillic
Copy link
Author

Done 👍

For context, the alternative to this PR is to add a lower bound in Project.toml to Julia v1.1. But since the previous release did not have this restriction, Pkg would just choose the old version in v1.0 and still crash.

@schillic
Copy link
Author

Hm, okay, it seems that the fix was not that easy. Admittedly I did not run the tests 😓
@KlausC, what is your opinion how to continue? I would go with option 1 below.

  1. I add a check around the broken tests to not be tested in v1.0.
    (Although the package has a different interface, I think it can still be useful in v1.0.)
  2. I undo the changes and instead require Julia v1.1 in Project.toml.
    (This will not really help as I explained above, but at least it would be clearer to people coming here and seeing that v1.0 is actually not supported.)

@KlausC
Copy link
Owner

KlausC commented Aug 28, 2023

As you can see, some tests fail for for long unsupported julia v1.0. I think it is not worth while diving into that.
Probably changing in Project.toml the compat entry to 1.2 would be best fix.

@schillic
Copy link
Author

I would be willing to spend the 5 minutes to have what I consider a proper fix. Otherwise it feels unsatisfactory to me. But if you prefer to not have it that way, I can go with my option 2.

@KlausC
Copy link
Owner

KlausC commented Aug 28, 2023

If we could agree, what a proper fix is, I would be ok with that.
The problem seems to be, that the types of the output differ in juliav1.0 and maybe 1.1.
It is not a good solution to ignore failing tests in that cases, IMO.

@schillic
Copy link
Author

Sounds good. Let me take a look at it. I will get back, either with a good solution or to go with option 2.

@schillic
Copy link
Author

The problem is that Pkg had a very different behavior in that old version, so writing generic code is hard.
I ended up copying the old code and rewriting it for v1.0.
I also added an additional @static in one place.
Coverage should be 100%.

@KlausC KlausC mentioned this pull request Aug 29, 2023
@KlausC
Copy link
Owner

KlausC commented Aug 29, 2023

I included your proposed functionality in the new release v0.3.3, which is available as of now.
Your PR may be closed.

@KlausC KlausC closed this Aug 29, 2023
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