-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
RFC: Centralize naming convention #7142
Comments
Also, imo, |
Testing mention @flatbuffers/maintainers |
Thanks @CasperN, I think this would be a good utility class to have. Class name might be I would make overloads for each of the methods that take in a How do we handle:
|
Yes, something like this would refactor the existing code, and centralize "how to make something camelcase" etc in one place. Then any overriding flags are suddenly simple to implement. Each language would instantiate a default. Generally sounds good to me? Of course a fair bit of work to do this, and I am not that convinced that these overriding options are that desirable, but I don't feel strongly either way. |
In the long run, it's not clear to me that we should be using strings everywhere. There's a semantic difference between a fully qualified type, However, that would be a different mass refactoring, so I agree that we should support
It seems reasonable to me. I can add
sgtm, I can add
I was thinking we'd have a factory fn per language. flatc.cpp would
Fair enough, I am 25% motivated by the case overriding FR, and 75% motivated by simplification/consistency |
I'm not sure where the best place for this is, but there also tends to be a bit of statefulness that can be needed to account for some things. E.g., I encountered a situation where for typescript if I created a
The generated code ends up with a constructor for
which results in a name conflict because within the context of the constructor, |
That's a good point, and there's a similar shadowing issue in #6845.
I think always using fully qualified types might be the way to go... but does that actually solve your problem if |
Fully qualified names don't resolve the issue I was noticing--but it could simplify the logic around the already-existing code to handle this with imports. To resolve what I was noticing, maybe there's some fancy typescript syntax to help (despite mostly talking about typescript here I don't actually develop in typescript much...); alternatively, you could enforce a naming convention for the generated members of the object API that guarantees that they will be different from any actual type names, but I don't know if that is feasible or desirable, and keeping track of what conventions everything is generated to follow to guarantee that they don't overlap sounds cumbersome. |
#7143 is another casing related bug: Two fields are different in the schema but, due to case-style normalization, generate the same symbol, which is invalid. |
Looks like @tira-misu found
|
@dbaileychess would you like to claim some subset of code generators to refactor? I don't mind doing it myself, but help is appreciated. |
I'll take cpp/c sharp/lua/json schema/fbs/text |
More relevant stuff #7156. What's with the recent surge in this family of bugs? 🤔 |
More thoughts:
Aside from addressing the keyword escaping bugs and enabling non-google style guides, I think this effort will at least make a dent in making the codebase more consistent |
SG
We can tackle that later.
Can you make it a template that takes anything that has a
Well, it needs someway to know where the breakage occurs in the input string to output to all the other types. I noticed this complexity too when switching to ConvertCase. Its painful now, but I think it is for the better.
Yes, our coverage is spotty and that makes refractorings harder. One way you can test is it to modify each function (
Yep, I hear you.
Since we are escaping correctly in the namer, can we just remove all explicit escaping in the generators? If there is a diff in the output, then those are probably real things we fix.
Yes, I would try to move all the odd cases into Namer so they are in one location. I am find with even
I would worry about those things later, lets just focus on the naming aspect.
100% |
That would work though I think I'd prefer to be more specific, e.g.
flatbuffers/src/idl_gen_swift.cpp Line 1914 in 2f84c60
Here's a fun one. Swift escapes keywords after converting case, which is great (I just need to add a config flag for this), but the casing is weird; it turns the variant name to lowercase before converting it to camel case. I guess I have to add a |
I can imagine! This has grown very organically, in many cases people finding spots in a language codegen that missed the right name generation long after the initial port.. nice to finally have that all centralized and cleaned up! |
TS done with #7488 |
After upgrading to the latest FlatBuffers version, I noticed that some generated names in Go were changed and I bisected that to #7150. In particular, field names like Our company has been using FlatBuffers for Go since 2016 and this is the first time AFAIK that we get a backwards incompatibility in the generated code, so this feels like a major issue for us. So I'm wondering if you consider this a regression to be fixed or is it just "tough luck, you should have followed the style guide"? Since we don't want to change all our own code which uses FlatBuffers, we'll probably work around this change by changing the field names in the schema to get the output that we want ( |
@jdemeyer this would be considered a bug, the namer work I started is not intended to be backwards incompatible, just standardize existing behavior which varies across languages by making it defined by the I think this might be resolvable by making @dbaileychess unfortunately, I don't really have the 20% time to finish this effort at the moment so you might want to reassign this one. |
Part of the problem is that the Go naming convention is UpperCamelCase with acronyms in all caps (for example ServeHTTP) and there is no real way to indicate that in snake_case: you would need to write |
This issue is stale because it has been open 6 months with no activity. Please comment or label |
This issue was automatically closed due to no activity for 6 months plus the 14 day notice period. |
Currently, the various code generators reinvent their own ways to escape keywords and recase identifiers to fit with local style.
DRY style concerns aside, the manual approach might not be working so well given #7137, #7138, #7139, #7140, and #7141. #7111 discusses overriding our casing conventions which would require touching almost everywhere, but would be easy given centralized naming.
In #7111 (comment) I outline a class that can be used to centralize naming conventions. I think something like:
I think of it similarly to data access objects (DAOs) except its for applying policies to our identifiers. This would be a pretty substantial change as it raises the abstraction level from "string manipulation code in every code generator" to "configuration per code generator", but I think it will help simplify our systems and make them more maintainable. What do we think?
@aardappel @dbaileychess @vglavnyy @krojew (is there an @all-maintainers?)
TODO:
Convert code generators
other stuff
The text was updated successfully, but these errors were encountered: