Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Adds an ident-<lang>-const command line option for each language. #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fostah
Copy link

@fostah fostah commented Aug 18, 2016

Also, adds support for camel case Djinni members being converted to underCaps. Previously an underscore was not added for each upper case letter.

…so, adds support for camel case Djinni members being converted to underCaps. Previously an underscore was not added for each upper case letter.
@smarx
Copy link

smarx commented Aug 18, 2016

Automated message from Dropbox CLA bot

@fostah, it looks like you've already signed the Dropbox CLA. Thanks!

@artwyman
Copy link
Contributor

A few comments:

  • I think you mean "ident" not "indent" in the title.
  • I feel like I'm not seeing a full set of changes. I see you adding new cmd-line args for const style, but I don't see any cases of them being used.
  • Can you explain more your intent with the under-caps style? In general, Djinni is built to assume that the names in the .djinni file itself are always in under_lower style, which is why it splits on underscores, before converting to the configured ident style. I'm not entirely against supporting different styles in the Djinni file itself, but it seems like trying to do so implicitly is going to be unpredictable, and doing it only for the underCaps style seems very limited. Seems totally fine for a private fork where you know which features you need to use and don't care about others, but not for the public version.

@fostah fostah changed the title Adds an indent-<lang>-const command line option for each language. Adds an ident-<lang>-const command line option for each language. Aug 19, 2016
@fostah
Copy link
Author

fostah commented Aug 19, 2016

I feel like I'm not seeing a full set of changes. I see you adding new cmd-line args for const style, but I don't see any cases of them being used.

When you say, "...cases of them being used." Are you referring to within Djinni itself or as part of the examples/test-suite? If you mean the former, Djinni already supports customizing const style, it just didn't expose it through the CLI options. So no further changes were needed.

For background on the camel case support: we weren't aware that the Djinni interfaces were only suppose to include under_lower style. We defined all our interfaces with camel case. This worked fine for everything but our C++ and Java constants, where we would SEECONSTANTSNAMEDLIKETHIS. Hence the change to underCaps specifically.

I can remove the underCaps change. However, looking at IdentStyle, it would be easy for me to add support for camel case in the Djinni interfaces to the rest of the style options, if you're open to it. camelUnder and camelLower already support it, and the change to underLower and underUpper would be very similar to what I did for underCaps.

@artwyman
Copy link
Contributor

Oh, I didn't realize const style customization was an internal feature you were simply exposing. Makes more sense now, thanks.

Which other ident styles were you using in Java and C++? There's definitely an assumption that the names in IDL are under_lower, which you can see in the various ident style definitions, e.g. the fact that underLower() simply returns its input, or that underUpper() splits on underscore.

I'm not averse to supporting different styles in the IDL file, but I feel like it should be done in a way which supports all options, not just some of them, which might mean a separate command-line arg to tell Djinni what input form to expect.

Are there specific styles in generated code weren't able to support using under_lower in IDL and some combination of Djinni arguments? Or do you just prefer camelCase names in your IDL files? If the latter, do you use them for everything (including class names), or just for "variable" names?

@fostah
Copy link
Author

fostah commented Aug 23, 2016

The choice to use camelCase was a preference and we use it for everything in our Djinni interface files. The only problem that it caused was with underCaps. However, we don't leverage underLower and underUpper.

I think supporting all the generated code styles in the Djinni interfaces would be possible, without a separate command-line argument. I would like to, at least, take a crack at it.

For this pull request, though, I think it would make sense to revert the camelCase support changes and just focus on exposing the const style customization. They're not related and I don't want the const style change held up while I experiment with the Djinni interface style support.

@artwyman
Copy link
Contributor

I like the idea of splitting this PR, to land the const options, and work on the variable styling separately. If you have an idea of how to support multiple styles cleanly without an argument I'm open to it, though I'm a bit wary of embedding too many assumptions such as the split-on-underscore-else-on-capital in this PR. It think it might be better to be explicit, and support all styles if you're going to support more than one.

Copy link
Contributor

@artwyman artwyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invoking code review flow now that it's available. Per the prior conversation, I think the next step is is for @fostah to remove the generator changes for input style from this diff, to allow the constant style arguments to be merged independently and leave the broader input style issue to be addressed separately.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants