dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
153 stars 43 forks source link

Naming of methods and constructors #585

Closed HosseinYousefi closed 1 year ago

HosseinYousefi commented 1 year ago

Java classes can have multiple (unnamed) constructors. Currently we name the other constructors as ctor1, ctor2, ... . I propose changing the names to new1, new2, ... .

Rationale: Currently if we have multiple Java methods with the same name like foo, we name them as foo, foo1, foo2, ... meaning we could write:

final foo = clazz.foo;
final foo1 = clazz.foo1;
// ...

Similarly we could

final constructor = clazz.new;
// but currently
final constructor1 = clazz.ctor1;
// after this change
final constructor2 = clazz.new2; // similar to clazz.new

@dcharkes @mahesh-hegde @lrhn Any better idea for naming multiple constructors / methods instead of the added number? Do you like ctor or new better?

lrhn commented 1 year ago

If you have foo1, foo2, etc. for methods, then new1, new2, ... seems reasonable for constructors.

The next question is which constructor gets to be just Foo.new. If the Java class has a zero-argument constructor, it's the obvious choice, but if it doesn't, should there be no Foo.new constructor in the Dart API? (Because users might start assuming that you can alway do new Foo(), if it's almost always possible.)

An alternative could be going by number of arguments, and have .new1 for a one-argument constructor, new2 for a two-argument constructor, and then go to new1a, new1b if there are multiple ones. For a few, fairly distinct constructors, that might be useful. If there are many constructors with the same number of arguments, it's just more verbose than new1, new2, etc. And it's different from what you do for methods, so if you do something else, you should probably do it everywhere.

(Would you worry about sorting in APIs if you get above new10? Would a letter be better, or at least postpone the problem to when you get above new2z?)

A completely different approach could be to try to derive a name from the parameter type, like Foo.fromInt, Foo.fromIntxInt. That may get very, very verbose for long parameter lists, at which point Foo.new17q starts looking reasonable. Again, only makes sense if you try the same for methods.

I definitely don't like ctor. Dart style guide says to avoid abbreviations. I also think constructor is too damn long.

The new1 is both close to something meaningful, and weird enough that it's clearly not trying to be a Dart name. I'd go with that.

HosseinYousefi commented 1 year ago

I definitely don't like ctor. Dart style guide says to avoid abbreviations. I also think constructor is too damn long.

Agreed.

An alternative could be going by number of arguments, and have .new1 ...

That would create a lot of unnecessary leading digits for constructors and methods that otherwise don't even have a same-named counterpart.

A completely different approach could be to try to derive a name from the parameter type, like Foo.fromInt, Foo.fromIntxInt. That may get very, very verbose for long parameter lists.

Yes, especially since Java is not known for short parameter type names either: Foo.fromAbstractBarBuilderFactory!

HosseinYousefi commented 1 year ago

One general problem with numbers is that they can move around when for example the new version of the library comes with added methods. And users must update their code to reflect this change. We even had this issue before: https://github.com/dart-lang/native/issues/696

One way of fixing this is to use some hashed version of argument names at the end of the method name. This is 1. ugly and 2. Requires ALL methods to end with this hash.

dcharkes commented 1 year ago

In FFIgen we've also gone with the numbering, so far we've not had too many issues with renumbering there. Though it might be that it's more common in Java libs?

A completely different approach could be to try to derive a name from the parameter type, like Foo.fromInt, Foo.fromIntxInt. That may get very, very verbose for long parameter lists, at which point Foo.new17q starts looking reasonable. Again, only makes sense if you try the same for methods.

This is what I use in the generated tests for FFI as well. But it's ridiculously long method (and in that case class) names. Those long names really only work in that situation because all the call-sites are generated as well.

Not sure if hashes are better or worse. 🙊 But at least both approaches are stable.

Maybe some kind of compromise is the name-postfix / hash from the moment we see two method names. In that case there's only ever 1 migration for users, instead of multiple as with renumbering.

A completely different approach would be to read in the yaml file that we generate, and use that to keep the renumberings from jumping around. (However, that makes JNIgen unstable if it is run on the same API in a clean project later...)