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

API Simplification #6

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

poncito
Copy link

@poncito poncito commented Jun 30, 2023

Hey Ondrej,
This is a PR to simplify the API of your package.
I advise to first read example/addressbook.jl, to see the potential benefit. We may want to start agreeing on this.
Then, we can iterate on the actual implementation.

There are a few things that I'm not sure I like about the proposed API:

  • enum comparison: I currently use something like Capnp.isenumequal(phone.type, :mobile). This is linked to this discussion. I'm not sure yet what the best solution is. We could also do phone.type == :mobile, but the community seems to not be in favour of that.
  • in the following statement bob.employment.unemployed, the second getindex is actually some kind of setter. There is certainly a way to do better.
  • I proposed to create a list using the parenthesis operator: bobPhones = bob.phones(2). Let me know if you agree.

About the implementation:

  1. the idea is simply to wrap your pointers in a struct for each julia type. Those types are defined in typed_pointers.jl.
  2. I wrote a small AnonymousEnums to deal with enums, without requiring the users to know about the names of the enum instances.

Also, even if this PR seems to work, there are some mistakes:

  1. the typed pointer are defined several times.
  2. only the generated types should be put in some module (possibly the optionnal c+ namespace). The implementations should be put somewhere else. This is to allow for a type that is called "Base" or "Core". This is why I am not a big fan of emulating the c++ namespacing with Julia modules. I'm not sure this is robust enough, and if the c++ annotations should be supported at all by this library.

Before fixing those mistakes, we may want to test this library. I suggest using this file.

@poncito
Copy link
Author

poncito commented Jun 30, 2023

Sorry, I forgot to emphasize this, but if we set the API first, then we can start writing tests, independently from the implementation.

This reverts commit ac1513f.
@poncito
Copy link
Author

poncito commented Aug 3, 2023

Hey, I've just reverted the last commit.

@poncito
Copy link
Author

poncito commented Aug 3, 2023

I'm not quite sure what to do for enums.

But I can make a preliminary remark. The current implementation for Base.which, that allows to discover the actual type of a union, is not optimal. It converts an integer into a symbol, and then we test for symbol equality.
With AnonymousEnums, even if we write "myenum == :mysymbol", the symbol gets propagated, and we compare integers, which not only is more efficient, but also allows to generate jump tables.

What I could propose, if we want to keep the syntax "reader.myenum == :myinstance" would be to wrap myenum into a TypedPointer that is meant to represent a symbol, but that actually contains an anonymousenum. This way we could keep the == syntax as specificity of this library. This would make the API simple, and the if/elseif/end blocks more performant.

@OndrejSlamecka
Copy link
Owner

OndrejSlamecka commented Oct 15, 2023

Good stuff, this makes it actually look like Julia. I think the getproperty/setproperty (dot syntax) is uncontroversial and I'll aim to look at that code soon.

Regarding some of the details:

  • enum comparison: I'd propose we keep the current style (Person_employment_union_employer) until we find a truly satisfying solution. Let's move this to another PR? (That said, I think the AnonymousEnums+isequal overload solution looks pretty neat.)
  • "bob.employment.unemployed, the second getindex is actually some kind of setter" -- could we do bob.employment.setUnemployed()? I'd like this to be a verb. Hopefully returning a function from getproperty on bob.employment.setUnemployed isn't considered too nasty and can be made efficient. (Or we could do bob.employment = Person_employment_union_unemployed for now if setUnemployed() somehow depends on the enum business above.)
  • "create a list using the parenthesis operator: bobPhones = bob.phones(2)" I'd like this to be a verb if you don't mind, bob.initPhones(2). (Edit: from a personal conversation, an option that would reflect setproperty! syntax, initList!(bob, :phones, 2))

Let me know how does that sound to you.

@jsimomaa
Copy link

jsimomaa commented May 7, 2024

Any plans regarding this pull request? The proposed API seems very nice!

@OndrejSlamecka
Copy link
Owner

Any plans regarding this pull request? The proposed API seems very nice!

I don't have any immediate plans with this but I am happy to review PRs if you want to take over this proposal.

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