Skip to content

Conversation

@mayssamm
Copy link
Member

@mayssamm mayssamm commented Aug 5, 2022


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • [X ] Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • [ X] Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

@yonzhan
Copy link
Collaborator

yonzhan commented Aug 6, 2022

Communication

@yonzhan yonzhan added this to the Aug 2022 (2022-09-06) milestone Aug 6, 2022
Copy link
Contributor

@kairu-ms kairu-ms left a comment

Choose a reason for hiding this comment

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

Please follow this guideline for command naming

Comment on lines 120 to 127
helps['communication identity create-user'] = """
type: command
short-summary: "Craetes a new ACS identity."
examples:
- name: create-user
text: |-
az communication identity create-user
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename communication identity create-user by communication identity user create

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

az communication identity create-user
"""

helps['communication identity delete-user'] = """
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename communication identity delete-user by communication identity user delete

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 211 to 218
helps['communication chat list-threads'] = """
type: command
short-summary: "Gets the list of chat threads of a user."
examples:
- name: chat list-threads
text: |-
az communication chat list-threads
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename communication chat list-threads by communication chat thread list

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 220 to 227
helps['communication chat create-thread'] = """
type: command
short-summary: "Creates a chat thread."
examples:
- name: chat create-thread
text: |-
az communication chat create-thread --topic "chat-topic"
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename communication chat create-thread by communication chat thread create

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

az communication chat delete-thread --thread-id "19:a-bcd=xyz"
"""

helps['communication chat list-participants'] = """
Copy link
Contributor

Choose a reason for hiding this comment

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

Same advice for renaming

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 133 to 135
c.argument('thread_id', options_list=['--thread-id', '-t'],
type=str, help='Thread id')
c.argument('message_id', options_list=['--message-id', '-i'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you always add -* for all arguments? It's not recommended. Those short argument alias may conflicted with the global arguments. Please add them only when needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed short arguments for most commands.

Comment on lines 28 to 30
with self.command_group('communication phonenumbers', client_factory=cf_communication_phonenumbers, is_preview=True) as g:
g.communication_custom_command('list-phonenumbers', 'communication_list_phonenumbers', phonenumber_arguments)
g.communication_custom_command('show-phonenumber', 'communication_show_phonenumber', phonenumber_arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this command group changed form GA to Preview again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed back to GA.

g.communication_custom_command('send-sms', 'communication_send_sms')
sms_arguments = ['connection_string']
with self.command_group('communication sms', client_factory=cf_communication_sms, is_preview=True) as g:
g.communication_custom_command('send-sms', 'communication_send_sms', sms_arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use send instead of send-sms

Copy link
Member Author

Choose a reason for hiding this comment

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

This was already released. Added 'sms send' and marked this as deprecated.

g.communication_custom_command('show-phonenumber', 'communication_show_phonenumber')
phonenumber_arguments = ['connection_string']
with self.command_group('communication phonenumbers', client_factory=cf_communication_phonenumbers, is_preview=True) as g:
g.communication_custom_command('list-phonenumbers', 'communication_list_phonenumbers', phonenumber_arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use list instead of list-phonenumbers

Copy link
Contributor

Choose a reason for hiding this comment

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

However as this command already release, you can keep them. But they are availed the naming guideline of azure-cli

Copy link
Member Author

Choose a reason for hiding this comment

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

Marked as deprecated as well.

c.argument('userid', options_list=['--userid', '-u'], type=str, help='ACS identifier')
c.argument('scopes', options_list=[
'--scope', '-s'], nargs='+', help='list of scopes for an access token ex: chat/voip')
c.argument('user_id', options_list=['--user-id', '-u'], type=str, help='ACS identifier')
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a breaking change to change option names from '--userid', '-u' to '--user-id', '-u'

Copy link
Member Author

Choose a reason for hiding this comment

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

Rolled back this renaming.

@kairu-ms kairu-ms merged commit aec2853 into Azure:main Aug 17, 2022
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ communication ] : https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1785968&view=results

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.

5 participants