Feature request: shorthand syntax for "each method only once"

[tomokinakamaru]

Feature request: shorthand syntax for "each method only once"

[tomokinakamaru]

Thank you for the deep insights :)

I was thinking of Option 1 at the time of my previous comment, but, as you pointed out, Option 4 would be more understandable for the library developers.

I don't find any problem in "the best approach" (= default limit + overriding with command argument), so I will implement the feature in that way. I will also follow your suggeation on the default limit. (500 seems good to me too.)

[bannmann]

Sounds fine!

[tomokinakamaru]

I totally agree that such a shorthand syntax will benefit library developers, so I will implement it soon.

One thing I concern is the number of generated class definitions. Since an order-free rule of N methods produces 2^N classes, I think Silverchain should print a warning message (or ask whether to continue or not; or throw an error?) before generating Java files when N is large.

[bannmann]

First of all: what exactly should the limit measure? I can imagine several options:

1. number of methods in each group (check each N individually)
2. number of classes generated by each usage of this feature (check each 2^N individually)
3. number of classes generated by all uses of this feature (2^N + 2^O + 2^P + ...)
4. total number of classes generated by Silverchain, regardless of features used or rule structure

My choice would be 4. I think that the alternatives are too vague as they only affect the output indirectly. Option 4 makes it really easy for the library developer to understand the parameter, manually check the current value (= ask OS for file count in output dir) and decide on an acceptable value. Furthermore, limiting the total number of classes would also cover other scenarios or even future Silverchain features that can also result in a large number of classes.

Regarding the behavior: I think the best approach would be to have a default limit that can be overridden with an optional command line parameter. If the effective limit is exceeded, Silverchain should fail with an error message like "Class limit (X) exceeded - this AG file would result in Y generated classes. If this is intentional, use the --max-class-count parameter to increase the limit."

This leaves the question of what the default value should be. For my API, Silverchain currently generates 88 .java files in total. I consider my API to be a rather small / simple one (compared to e.g. jOOQ), so my guess is that something like 500 might be a good default.

[tomokinakamaru]

While working on this feature, I found that ~ is not an appropriate notation.

The operator ~ would be a binary operator like +, so foo() ~ bar() ~ baz() should be the same as (foo() ~ bar()) ~ baz(). However, if foo() ~ bar() means foo() bar() | bar() foo(), then (foo() ~ bar()) ~ baz() means

• foo() bar() baz() or
• bar() foo() baz() or
• baz() foo() bar() or
• baz() bar() foo(),

which is not what we need (there should be 3x2x1 = 6 rules). Although the two expressions foo() ~ bar() ~ baz() and (foo() ~ bar()) ~ baz() can mean different things, it would be a bit confusing (at least for me).

I think we should not use a binary operator here. My idea is to use the notation { <rule>, <rule>, ... } to list the choices. For example,{ foo(), bar(), baz() } is expanded into 6 rules by Silverchain. With this syntax, your case is expressed by { A()?, B()?, C()? }.

What do you think about this notation? Any kinds of opinions are more than welcome! (I have pushed the draft implementation to the development branch.)

[bannmann]

Right, a binary operator doesn't make sense.

My first reaction reaction to using curly braces was the thought that Silverchain should use something else because they were already used for enclosing the rules of one builder class. However, I realized that one could argue that in both cases, the curly braces list several options (chain rules vs methods/expressions) which have no strict order, and that the result is kind of the "sum"/"permutations" of everything inside (state classes modelling the rules vs rules for all permutations). Also, I couldn't come up with another pair of ASCII characters that would work equally well yet are not used for something else.

So all in all, I think the suggested notation is memorizable and should work fine for library developers.

I tested the develop build and it worked fine. It's also great that you managed to make the feature more powerful than the one I originally asked for:

• One can leave out the ? to require calling all methods, but allow any order (as you demonstrate with the Triplet Builder test case in the code)
• The elements of the order-free group can be more complex than a simple A()?. As an experiment, I managed to generate an API that allows calling the "only once" methods A/B/C in between calls of other, unrestricted methods X/Y/Z:

  (X() | Y() | Z())*
  {
      ( A()? (X() | Y() | Z())* ),
      ( B()? (X() | Y() | Z())* ),
      ( C()? (X() | Y() | Z())* )
  }
  

The only trouble I had was that the error message when one has an extra comma after the last group element is not pointing at the right location (at the start of the group instead of the end). This led to much head-scratching until I isolated the cause.

[tomokinakamaru]

Thank you for reviewing the feature :)

The only trouble I had was that the error message when one has an extra comma after the last group element is not pointing at the right location (at the start of the group instead of the end). This led to much head-scratching until I isolated the cause.

I tested several patterns with extra commas, but I couldn't reproduce the problem :( I will investigate the cause more deeply if you share your case. (Probably, the problem is caused by the messy grammar definition in silverchain.jj.)

Anyway, to avoid such confusion, I fixed the grammar definition to allow an extra comma at the end of the last element. So the error message problem does not happen again (hopefully). I think we can

• Ignore the error message problem for now,
• close this issue after merging the new syntax (in develop) into master, and
• open another issue if a similar problem is observed.

Do you think of a better workflow? Any suggestions are more than welcome :)