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

Naming convention for names containing acronyms in standard modules #20240

Closed
daviditen opened this issue Jul 18, 2022 · 18 comments
Closed

Naming convention for names containing acronyms in standard modules #20240

daviditen opened this issue Jul 18, 2022 · 18 comments

Comments

@daviditen
Copy link
Member

What should be the naming convention for names containing acronyms/initialisms? It was discussed briefly in #6698 (comment) and #17936 but there wasn't a conclusion.

The example given there was: How should render_html_dom() be camel-cased? renderHtmlDom() or renderHTMLDOM()?

Should there be special rules for short acronyms? Acronyms at the end the symbol name? Any other special cases?

@bradcray
Copy link
Member

Thanks for opening this, David! FWIW, this question drives me bonkers (in that I don't feel like there's a good answer).

My understanding (based on what others tell me) is that Python preserves capitalizations of acronyms but that Microsoft does not (?). I'd also be curious what the Julia/Swift/Rust positions on this question would be.

David, in your example, it's not obvious what would cause us to use DOM rather than Dom—isn't that just an abbreviation? (in which case I'd expect the second choice to be renderHTMLDom().

Acronyms at the end the symbol name?

If we didn't preserve capitalization earlier in the identifier, I'd be open to granting exceptions for capitalizations at the end of identifiers personally. It's inconsistent, but with good reason (there's no ambiguity as to where the acronym stops).

@bradcray
Copy link
Member

One piece of guidance we might potentially agree upon easily is that if acronyms can be replaced with a short-ish word, the word may be preferable. For example, I think we recently replaced eol with newline somewhere (?) which felt like a net win to me.

@daviditen
Copy link
Member Author

David, in your example, it's not obvious what would cause us to use DOM rather than Dom—isn't that just an abbreviation?

In the example that I took from #17936 (comment) DOM stands for Document Object Model so I'd expect it to be all capitalized if written by itself.

@bradcray
Copy link
Member

bradcray commented Jul 20, 2022

I just ran into an interesting case to me, which was imagining an argument named localeID (or fooID) and wondering if I'd need to write it as localeId / fooId (which causes the I to be read as a lower-case L to me). Either the "exception for short acronyms" or "exception for terminating acronyms" clauses could address such cases if everyone preferred the ID versions.

@lydia-duncan
Copy link
Member

That's an interesting case, Brad. But would "our style guide states that all names use camelCase or PascalCase" be enough to recognize that it should be a capital I, since "localeld" doesn't obviously look like a single word?

@bradcray
Copy link
Member

To me, there are two issues here. The first is whether it's unfortunate that 'ID' would be written 'Id' which is also a word and short. The second is the potential confusion about I vs. i (though, as the GitHub formatting points out, with a serif font, that's less of an issue). The first part still stands, though.

@lydia-duncan
Copy link
Member

I think I'm a little confused. Isn't "id" here an abbreviation for "identifier"/"identity"/etc., not an acronym? Are you worried it will be confused with "id" from psychoanalysis? Or something else I'm not thinking of?

@mstrout
Copy link
Collaborator

mstrout commented Jul 25, 2022

I believe the dyno team picked blahAst for the compiler data structures that refer to Abstract Syntax Trees. We don't necessarily have to be consistent across the compiler written in C++ and our libraries, but we should at least be aware. @mppf , @dlongnecke-cray, or @arezaii do you have any input?

@dlongnecke-cray
Copy link
Contributor

dlongnecke-cray commented Jul 25, 2022

Dyno has AstNode, AstList, for things related to AST. For a given type ID, we prefer locateId(), idToAst(), etc.
Based on that my stance would be:

  • It's fine to say DOM when you're writing docs
  • It's fine for the type name to be DOM, but personally I'd prefer Dom or htmlDom/HtmlDom (ID is a bit inconsistent here, but perhaps it's OK because it's two letter and starts with i)
  • The routine name should be renderHtmlDom consistent camel case

I prefer routines to be consistent camel case. I can usually map back mentally to the acronym from the routine name or a formal type if need be. But personally I tend to stumble over mixedCASEIsConfusingME...

@mppf
Copy link
Member

mppf commented Aug 3, 2022

Yes, for the most part we are using (or trying to use) a style like renderHtmlDom / localeId in dyno. My view is that we started doing that as a branch prediction for how this discussion would go. I am pretty happy with this style based on that experience.

@daviditen
Copy link
Member Author

This was discussed at a module 2.0 re-review meeting yesterday. The team that discussed it voted between three main options (we also briefly discussed some other options like capitalizing short acronyms):

  1. Make all symbols with acronyms fully camelCase. renderHtmlDom, module io, var node: ast
  2. Make all symbols with acronyms camelCase unless the full symbol name is an acronym. If the full symbol name is an acronym then if it is a type that normally starts lowercase it should be all lowercase (records, functions, etc.). If it is a type that normally starts uppercase it should be all uppercase (e.g. module names like IO). renderHtmlDom, module IO, var node: ast
  3. Make acronyms in symbol names all uppercase. renderHTMLDOM, module IO, var node: AST

There was nearly even support for options 2 and 3. The people who supported option 3 said they were willing to accept option 2 so that is our choice. Michael is updating StandardModuleStyle.rst to include this choice.

@bradcray
Copy link
Member

bradcray commented Aug 5, 2022

Michael is updating StandardModuleStyle.rst to include this choice.

I don't object with the decision here per se, but just as a point of order, my understanding is that when we break into these subteams, that subteam doesn't have the power to make the final decision on the topic in isolation (since it's impossible for someone interested in multiple topics to attend them if they're occurring simultaneously), but instead owns the task of trying to create a consensus and then reflecting that choice back to the full group for objections or insights that they may have missed. Tagging @lydia-duncan as I think this is how she's explained the intended format in the past as well.

@bradcray
Copy link
Member

bradcray commented Aug 5, 2022

I'd also be curious for a readback on whether any discussion occurred around the comment I posted to chat once I knew I'd be missing the discussion: "I thought someone asserted lately that Python didn't do it that way, which made me wonder whether we were aligning with the wrong approach, and/or which other languages are using this approach." Specifically, are we diverging from Python, and if so, if a Python user complains to us about the choice, do we have an explanation for why we diverged, and is there a precedent to point to to assure them that we're not going off into the wild on our own?

@lydia-duncan
Copy link
Member

my understanding is that when we break into these subteams, that subteam doesn't have the power to make the final decision on the topic in isolation (since it's impossible for someone interested in multiple topics to attend them if they're occurring simultaneously), but instead owns the task of trying to create a consensus and then reflecting that choice back to the full group for objections or insights that they may have missed.

The subteam does have the obligation to give those that weren't present a last chance to object. However, if we're finding that too many of the topics are not getting resolved because we could not get all interested parties in the room, then perhaps we should move back to full group discussions.

@daviditen
Copy link
Member Author

daviditen commented Aug 5, 2022

I think Python's approach is to make acronyms all uppercase for class names and all lowercase otherwise.

This stack overflow post has the following example:

>>> import ipaddress  # IP is lowercase because this is a module
>>> ipaddress.IPv4Address  # IP is uppercase because this is a class
<class 'ipaddress.IPv4Address'>
>>> ipaddress.ip_network  # IP is lowercase because this is a function
<function ip_network at 0x0242C468>
>>>

We don't plan to follow python in this because it doesn't work well when using camelCase like we do instead of snake_case like python does. Java and Rust each only capitalize the first letter when acronyms occur in camelCase symbol names.

@daviditen
Copy link
Member Author

Option 2 from #20240 (comment) was the choice after notifying the whole team and giving time for any additional comments.

@bradcray
Copy link
Member

For posterity: In a discussion about numPUs() (vs. numPus()) and openFP (vs. openFp()) today, there were questions about whether adhering strictly to the style guidelines resulted in the better name (specifically, 8 people preferred numPUs() and 2 numPus()). That led to questions about whether there should be exceptions, such as:

  • should exceptions be permitted when the acronym is followed immediately by some other letter (like the plural 's' here) which breaks the normal ambiguity of "where to stop reading" when maintaining capitals? (e.g., numPUPerNode is problematic ("where does the word stop in 'PUP'?"), but numPUsPerCpu is arguably not since the 's' breaks up the identifier
  • should exceptions be permitted when reading the result reads as another English word than what was intended (in this case "pus" vs. "PUs"?
  • should exceptions be permitted for short acronyms?
  • should exceptions be permitted when the acronym comes at the end of the identifier?
  • should exceptions be permitted based on the authors'/group's druthers?

And then a related follow-up question was: If we did permit one of these exceptions, are we requiring that the exception be applied? (e.g., if we used 'numPUs' based on the "plural" argument, would that mean that a query asking about the number of VIP people would be written numVIPs, or could it still be written numVips() if the author preferred?

There was also a question about whether even considering such questions was inappropriate since it seemed to be reversing a decision that had been made (where, personally, I don't view these proposals as being reversing so much as refining). I also don't think we should be slavish to decisions we've made if, upon applying it in practice, we end up with results that we don't actually like. I.e., we should be able to question or refine decisions if there are significant hesitations, so long as it doesn't result in thrashing.

@lydia-duncan
Copy link
Member

Cross post from here, but figured I should also keep this with the main conversation:

I think it's reasonable to just treat numPUs as an exception if we do stabilize with that name, but if we end up with 5 or more exceptions to the current rule, I think then is the time to think about if we should make a different rule or a subrule that can reduce us back down to a reasonable number of exceptions

mppf added a commit that referenced this issue Sep 26, 2022
…e-fac

Update standard module style guide

This PR updates StandardModuleStyle.rst per discussions in issues #14291
and #20240. We might change our mind about the capitalization rule but #20699
will add to this PR for that & it's reasonable to have the general
guidance written down even if we are going to adjust it in the future.

Reviewed by @lydia-duncan and @daviditen - thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants