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

Import Base.@kwdef for compatibility with Julia versions below 1.9 #390

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

sambuddhac
Copy link
Contributor

This is a minor code edit to fix the failure to precompile error. i have replaced the @kwdef with Base.@kwdef, since otherwise the code won't precompile (and consequently, work)

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.67%. Comparing base (a0fe596) to head (6a2ab6b).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #390   +/-   ##
=======================================
  Coverage   77.67%   77.67%           
=======================================
  Files          68       68           
  Lines        5289     5289           
=======================================
  Hits         4108     4108           
  Misses       1181     1181           
Flag Coverage Δ
unittests 77.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/InfrastructureSystems.jl 90.00% <ø> (ø)

Copy link
Contributor

@GabrielKS GabrielKS left a comment

Choose a reason for hiding this comment

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

@sambuddhac what version of Julia you are using? @kwdef gets exported from Base as of 1.9 and so this works without the change for me, so I am assuming earlier than that.

Anyway, my preference for supporting versions earlier than 1.9 would be to import @kwdef. Adding this line to the imports at the top of src/InfrastructureSystems.jl should work:

import Base: @kwdef  # for Julia versions earlier than 1.9

and then prefixing with Base shouldn't be required:
Screenshot 2024-07-31 at 1 35 31 PM

Thanks for catching the compatibility issue.

@sambuddhac
Copy link
Contributor Author

@sambuddhac what version of Julia you are using? @kwdef gets exported from Base as of 1.9 and so this works without the change for me, so I am assuming earlier than that.

Anyway, my preference for supporting versions earlier than 1.9 would be to import @kwdef. Adding this line to the imports at the top of src/InfrastructureSystems.jl should work:

import Base: @kwdef  # for Julia versions earlier than 1.9

and then prefixing with Base shouldn't be required: Screenshot 2024-07-31 at 1 35 31 PM

Thanks for catching the compatibility issue.

Thanks @GabrielKS . I am using Julia 1.8. In that case, would yall mind updating the README (and perhaps the doc pages too) with these set of instructions? The README for PowerSystems.jl at least says Julia 1.6+ is good. Thanks bunches !!!

Sam

@GabrielKS
Copy link
Contributor

Thanks @GabrielKS . I am using Julia 1.8. In that case, would yall mind updating the README (and perhaps the doc pages too) with these set of instructions? The README for PowerSystems.jl at least says Julia 1.6+ is good. Thanks bunches !!!

Sam

Ah, I meant that that this PR should add that import (rather than adding the prefix) such that it does work out of the box for 1.6+. I'd be happy to do that if you don't want to.

@sambuddhac
Copy link
Contributor Author

Thanks @GabrielKS . I am using Julia 1.8. In that case, would yall mind updating the README (and perhaps the doc pages too) with these set of instructions? The README for PowerSystems.jl at least says Julia 1.6+ is good. Thanks bunches !!!
Sam

Ah, I meant that that this PR should add that import (rather than adding the prefix) such that it does work out of the box for 1.6+. I'd be happy to do that if you don't want to.

@GabrielKS Oh, my apologies for the misunderstanding. I will be extremely glad to take care of this in just a couple hours (right now, driving home from work and then I need to take care of some personal stuff and then, will be quickly on this PR to fix it). Thanks so much and please allow me a couple hours or so for the PR revision.
Sam

@sambuddhac
Copy link
Contributor Author

Hi @GabrielKS , I just incorporated your suggestion. Once this is approved and merged, I will update the same edit on the other repos (following, successful testings). Thanks !!!

@GabrielKS GabrielKS self-requested a review July 31, 2024 23:09
Copy link
Contributor

@GabrielKS GabrielKS left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@GabrielKS GabrielKS changed the title Replaced @kwdef with Base.@kwdef since otherwise package errors Import Base.@kwdef for compatibility with Julia versions below 1.9 Jul 31, 2024
@sambuddhac
Copy link
Contributor Author

Looks good, thanks!

@GabrielKS thanks a lot !!! I unfortunately, don't have write access to the repository. Would you please merge? Or, alternatively, if you grant me write-access, I will be very happy to rebase/merge/squash-n-merge (on main or develop) whichever is your preferred modus-operandi. :)

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