Skip to content

Return enum elements for overlap, and adapt ITF tests #518

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

Merged
merged 2 commits into from
May 16, 2022

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Apr 23, 2022

No description provided.

@lbenet lbenet mentioned this pull request Apr 23, 2022
16 tasks
@lbenet
Copy link
Member Author

lbenet commented May 7, 2022

After today's meeting I've checked EnumX and it looks pretty cool.

  • Considering adding it as a new dependence seems ok to me, in the sense that it is a pretty light package.
  • Using it would allow to export only the typename (Overlap is perhaps better than OverlapType), which brings along the different (enum) instances.
  • We could get rid of the odd naming I proposed initially, e.g. _contains or _first_empty, in favor of Overlap.contains or Overlap.firstEmpty (as you suggest) or Overlap.first_empty.

@lucaferranti What's you opinion on giving it a try?

@lbenet lbenet added the 1.0 Planned for the major 1.0 release label May 9, 2022
@lucaferranti
Copy link
Member

@lbenet sorry for the delay in answering, I didn't notice the comment, I think we could give it a try, but does it have a significant advantage wrt returning a symbol?

@lucaferranti
Copy link
Member

well we can try, it's not a biggie :)

@lbenet
Copy link
Member Author

lbenet commented May 13, 2022

I tested using EnumX.jl: it corresponds to what we have in this PR, with a nicer interface, in the sense that only Overlap (the struct containing the overlap instances) is exported, instead of all specific instances. Shall I push the commit here so you can have a look and test it?

@lucaferranti
Copy link
Member

@lbenet sure!

@lbenet lbenet force-pushed the lb/enum_overlap branch from 2064aae to d5e48ac Compare May 14, 2022 14:42
@lbenet
Copy link
Member Author

lbenet commented May 14, 2022

I think this is ready @lucaferranti. Shall we backport it in an adapted form to master?

@lucaferranti
Copy link
Member

@lbenet LGTM, merging. don't have strong opinions on whether or not we should backport this to master and release a new patch release. If 1.0-dev is going to be merged "soon", then I think we can wait.

@lucaferranti lucaferranti merged commit f385b7f into 1.0-dev May 16, 2022
@lucaferranti lucaferranti deleted the lb/enum_overlap branch May 16, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Planned for the major 1.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants