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

Node.js doesn't name it's operation classes the same as other languages #1491

Closed
matthchr opened this issue Oct 5, 2016 · 4 comments
Closed

Comments

@matthchr
Copy link
Member

matthchr commented Oct 5, 2016

In C#, the operations classes and properties seem to be named consistently like so (assume no conflicts currently):
Swagger:
BatchAccount_Create - an operation

C#
class BatchAccountOperations - the class containing the operations associated with a Batch account
BatchClient.BatchAccount - a property on the client

Python
class BatchAccountOperations(object) - the class containing the operations associated with a Batch account
BatchClient.batch_account - the property on the client

Node
function BatchAccount(client) { - the class containing the operations associated with a Batch account
BatchClient.batchAccount - the property on the client

Now if we add in a model type to our Swagger that conflicts with the operation group name in the Swagger spec we get the following changes:
BatchAccount - a model in the Swagger specification

C#: No changes
Python: No changes
Node.js: Renames the class and the property to be BatchAccountOperations and batchAccountOperations respectively.

This is problematic because it easily leads me to a client that looks like this:
client.fooOperations, client.bar, client.bazOperations, client.qux (i.e. my properties aren't consistently named, either with an Operations suffix or otherwise).

This leads me to my real question:
Why doesn't node just do what C# and Python do, and always append operations on the class name (it can append it or not on the property name, as long as it consistently does so). To me this seems to have a few benefits:

  1. It more accurately describes what the class actually is (it really is a class that is a collection of operations, it is not a BatchAccount as in my example above)
  2. It increases the likelihood that people don't accidentally end up with an inconsistent API in Node
  3. It also matches the other languages (not that big of a deal, but nice)

The downsides of this:

  1. It's a big breaking change for most clients I would assume

If you're not willing to accept this change, here are some other ideas that might help alleviate this issue...

  1. Make the linter warn about duplicate model and operation group names
  2. Make the AutoRest warning that is emitted when something is renamed due to a conflict an error that you must opt out of in the Swagger specification or on the cmdline of AutoRest?
@matthchr
Copy link
Member Author

matthchr commented Oct 5, 2016

@amarzavery @jasper-schneider
@tbombach -- Linter suggestion

@matthchr matthchr added the nodejs label Oct 5, 2016
@fearthecowboy
Copy link
Member

Hey @matthchr ... are you talking about the CSharp.Azure and Python.Azure generators? The non-Azure generators are kinda inconsistent with that too, and I wasn't sure that it is all that consistent in all the Azure-flavored ones either.

You're correct in that 'fixing' that now would be quite a bit of a breaking change.

I think we're going to have to bring this up for discussion; this is certainly something that I've noticed, and we're starting to get some inconsistency.

@matthchr
Copy link
Member Author

I thought the naming of the operation groups actually is part of the base generator, not the .Azure flavor.

But yes, I am talking about the inconsistencies in naming behavior between the languages, specifically the fact that node doesn't append Operations to them by default and everything else (that I know of) does.

My main complaint is that if every language does it's own thing with naming (and the subsequent conflicts) it becomes harder and harder to pick names to minimize conflicts in all languages. And then you get awkward looking interfaces in some languages like my team has for node now, where we have client.pool, client.job, client.taskOperations, client.fileOperations, etc. (Note the inconsistency on the client object with respect to what it's properties are named.

@fearthecowboy
Copy link
Member

Adding this as a feature consideration for vNext generators. (see #2448)

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

No branches or pull requests

2 participants