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

[Cosmos] Naming Consistency #5263

Closed
2 tasks
southpolesteve opened this issue Sep 26, 2019 · 4 comments · Fixed by #6110
Closed
2 tasks

[Cosmos] Naming Consistency #5263

southpolesteve opened this issue Sep 26, 2019 · 4 comments · Fixed by #6110
Assignees
Labels

Comments

@southpolesteve
Copy link
Contributor

Cosmos needs to alias some of our methods and options to match with other Azure SDKs. Some that have been identified already:

  • maxRetryAttemptCount -> maxTries
  • maxWaitTimeInSeconds -> timeoutInMs / timeoutInSeconds
  • FixedRetryIntervalInMilliseconds -> retryIntervalInMs
  • InMilliseconds -> InMs (in many places)

TODO:

  • Full API audit with @bterlson to expand on the above list
  • Alias old names to new names
@southpolesteve
Copy link
Contributor Author

Additional notes from our last meetings:

  • getPartitionKeyDefinition -> readPartitionKeyDefinition

  • Top level CosmosClient methods

    • get -> read
  • CosmosClientOptions

    • Should add pipelineOptions on move to core-http
  • De-pascal case everything external. If it is internal, does not matter

  • Actual constants -> SCREAMING SNAKE CASE if top level

  • InMilliseconds -> InMs everywhere

  • FeedOptions

    • continuation -> continuationToken
  • readChangeFeed -> items.changeFeed.read()

  • QueryIterator - Known issue. Make iterator a async iterator

  • Brian to follow up on accessCondition vs accessConditions

@xirzec
Copy link
Member

xirzec commented Nov 7, 2019

  • Top level CosmosClient methods
    • get -> read

There are only three such methods on CosmosClient, getDatabaseAccount, getWriteEndpoint, and
getReadEndpoint.

None of them sound particularly improved by this conversion, especially the last two: readDatabaseAccount, readWriteEndpoint, readReadEndpoint

  • CosmosClientOptions

    • Should add pipelineOptions on move to core-http

This seems like longer-lead work than simple renames. Is it really something we want to bundle with the other work?

  • De-pascal case everything external. If it is internal, does not matter

I'm not seeing where we have PascalCase methods/properties?

  • Actual constants -> SCREAMING SNAKE CASE if top level

Is this referring to things like const WindowsInterruptedFunctionCall = 10004; ?

  • readChangeFeed -> items.changeFeed.read()

I'm confused about this one. Is the idea that readChangeFeed() becomes a getter named changeFeed that still returns a ChangeFeedIterator? Does something in ChangeFeedIterator need to change so it has read instead of fetchNext ?

  • Brian to follow up on accessCondition vs accessConditions

@bterlson ?

@xirzec
Copy link
Member

xirzec commented Nov 8, 2019

I'd also appreciate if @southpolesteve or @bterlson could clarify what is meant by 'aliasing'.

For example, I could leave the method getPartitionKeyDefinition and have it simply return the result of calling readPartitionKeyDefinition, but it's subtler what we should do for interfaces.

Like for ErrorResponse if we rename retryAfterInMilliseconds to retryAfterInMs, do we have to go to every implementer and make sure both properties exist and are in sync? Except since it's an optional property, this is tricky to enforce, since it would still be valid to the interface if only one existed.

TBH, it feels weird to strive for backcompat here, either take a major version bump or abandon consistency.

xirzec added a commit to xirzec/azure-sdk-for-js that referenced this issue Nov 8, 2019
Fixes Azure#5263 by updating names to align better with other packages.
@xirzec
Copy link
Member

xirzec commented Nov 12, 2019

Notes from chatting about this:

xirzec added a commit that referenced this issue Nov 18, 2019
* Update names in cosmos for consistency

Fixes #5263 by updating names to align better with other packages, but leave old names for backwards compatibility until the next major version bump.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants