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

Use concrete field type for type stability #2

Merged
merged 1 commit into from
Feb 3, 2020

Conversation

yha
Copy link
Contributor

@yha yha commented Feb 2, 2020

Makes everything inferrable and faster.

@yha
Copy link
Contributor Author

yha commented Feb 3, 2020

Tests seem to fail here, although they pass locally on my machine. I think the difference is in OffsetArrays version. I'll look into it.

@yha
Copy link
Contributor Author

yha commented Feb 3, 2020

The problem is that JuliaArrays/OffsetArrays.jl#90 added a custom axis type to the OffsetAxis union, so the ambiguities resolved in #1 crop up again. This breaks existing functionality from #1 (nothing to do with this PR).
I think now the proper solution would involve fixing the way offset arrays are designed in Base, since the current design seems to basically guarantee that different packages defining similar methods will trigger ambiguities when used together.
For now I suppose we can either add a dependency on OffsetArrays and disambiguate on OffsetArrays.OffsetAxis, or add a version constraint for OffsetArrays (but can a version constraint be used without a dependency?).

@Vexatos
Copy link
Owner

Vexatos commented Feb 3, 2020

I really wish there was a way to pull IdOffsetRange without having to add a hard dependency on OffsetArrays, at least for now. That is just really ugly and wasteful.

@Vexatos Vexatos merged commit de8c6c9 into Vexatos:master Feb 3, 2020
@Vexatos
Copy link
Owner

Vexatos commented Feb 3, 2020

This PR looks good unrelated to the OffsetArrays issue, which I actually made an issue for now. Thanks for your contribution, and for looking into this.

Vexatos added a commit that referenced this pull request Feb 3, 2020
Use concrete field type for type stability
@yha yha mentioned this pull request May 4, 2020
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