Skip to content
This repository has been archived by the owner on Jul 13, 2021. It is now read-only.

Fix compilation error on Julia 1.3 #687

Merged
merged 9 commits into from
Apr 14, 2021

Conversation

devmotion
Copy link
Contributor

Fixes MakieOrg/Makie.jl#904 which I also observed in https://github.com/devmotion/AbstractGPsMakie.jl/pull/1/checks?check_run_id=2309110134.

To avoid regressions or similar issues on Julia 1.3 I replaced the CI tests on Julia 1.4 with Julia 1.3. It seemed reasonable to test the oldest officially supported Julia version. I am not sure if there was a particular reason for running tests on Julia 1.4.

@devmotion
Copy link
Contributor Author

Tests on Julia 1.3 fail since only was introduced in Julia 1.4.

@devmotion
Copy link
Contributor Author

I replaced all occurrences of only with first since the implementation always checks if the container contains only one element anyway. Alternatively, one could load Compat.

@devmotion
Copy link
Contributor Author

The documentation build fails since it can't be pushed to MakieDocumentation. Apart from that, tests pass now both on Julia 1.3, 1.6, and nightly.

@greimel
Copy link
Contributor

greimel commented Apr 12, 2021

I wouldn't replace only with first since the two functions don't do the same thing. Luckily only is provided by https://github.com/JuliaLang/Compat.jl for older Julia versions.

I'd rather go for that if @SimonDanisch and @jkrumbiegel want to take this dependency.

@devmotion
Copy link
Contributor Author

I wouldn't replace only with first since the two functions don't do the same thing.

Sure, but AFAIK the main difference is that only throws an error if the collection contains not exactly one element. However, currently it is checked explicitly in the code of AbstractPlotting before every call of only that the collection contains exactly one element, so maybe it is OK to avoid this redundancy and replace only with first.

@SimonDanisch
Copy link
Member

I'd be ok with first, I suppose ;) @jkrumbiegel ?

@jkrumbiegel
Copy link
Member

Is Compat a heavy dependency? I'd prefer to stay with only via Compat if not, as it communicates a different thing than first. first wouldn't error with multiple elements which might make it harder to catch bugs based on wrong assumptions.

@greimel
Copy link
Contributor

greimel commented Apr 13, 2021

I believe it only depends on stdlibs: https://github.com/JuliaLang/Compat.jl/blob/master/Project.toml

(is this implied from the fact that there are no compat entries?)

@devmotion
Copy link
Contributor Author

Yes, it only depends on stdlibs.

As mentioned above, I am happy to add Compat instead of replacing only with first. That being said, I don't think there's a compelling reason to use only here since before every invocation of only the implementation checks explicitly that there is exactly one element and throws custom error messages otherwise, i.e., the safety checks and error messages of only are not used currently. You can see some of these checks in the Github diff above the modified lines.

@jkrumbiegel
Copy link
Member

Well if the code already checks for single element with a good error message, you might as well do arr[1]

I didn't check the specific instances of only

@devmotion
Copy link
Contributor Author

I already switched to Compat now 😁 Would you like me to revert it?

@jkrumbiegel
Copy link
Member

Yeah if it's such a small thing, let's not deal with the complexity of another dependency I'd say, even if it's just about compat bounds of Compat

@devmotion
Copy link
Contributor Author

OK, done 👍

@devmotion
Copy link
Contributor Author

Bump 🙂

@jkrumbiegel jkrumbiegel merged commit bd42686 into JuliaPlots:master Apr 14, 2021
@devmotion devmotion deleted the dw/julia13 branch April 14, 2021 20:42
@devmotion
Copy link
Contributor Author

Thanks! Is it possible to get a new release with this fix? I already bumped the version number in this PR 🙂

@jkrumbiegel
Copy link
Member

done

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Julia compat for AbstractPlotting
4 participants