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

[Eslint Plugin] Update ts-naming-options rule to expect a more reasonable name for constructor options #11403

Merged

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Sep 22, 2020

Currently ts-naming-options expects the type of the options parameter to the constructor of a client class to be named ConstructorOptions, but I think a more reasonable behavior would be to expect <class name>Options instead and this PR implements this. Suggestions for alternative behaviors are welcome!

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Seems very sensible to me.

I think @jonathandturner brought this up in one of our team meetings so you've got at least two votes (probably). :)

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

The change to constructor options naming feels much better

@deyaaeldeen deyaaeldeen merged commit befc822 into Azure:master Sep 25, 2020
@deyaaeldeen deyaaeldeen deleted the eslint-plugin-update-ts-naming-options branch September 25, 2020 14:55
deyaaeldeen added a commit to deyaaeldeen/azure-sdk-for-js that referenced this pull request Sep 25, 2020
…able name for constructor options (Azure#11403)

* better handling of class constructors in ts-naming-options rule

* adding tests

* adding upsert to my dictionary
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

Successfully merging this pull request may close these issues.

4 participants