search

Jericho / ZoomNet

.NET client library for the Zoom.us REST API v2
MIT License
68 stars 48 forks source link

Added default_password param to CreateMeeting APIs #280

Open mxswd opened 1 year ago

mxswd commented 1 year ago

If you want to make a meeting and have zoom pick a password you need to send this param. I've tested it and get back passwords in the JoinUrl.

https://developers.zoom.us/docs/api/rest/reference/zoom-api/methods/#operation/meetingCreate

Jericho commented 1 year ago

While I review your PR, please raise an issue to clearly explain the problem you are facing, describe the changes you intend to make, share the ideas you have, ask questions, etc. Normally, it's preferable to raise the issue first before working on the PR to ensure the project maintainer is interested in the changes you are proposing. It would be a shame for you to invest time and effort in writing code only to find out upon submitting your PR that there is a already a solution/workaround or that the maintainer is not interested in the solution you are offering.

There is another reason for raising issues that is specific to the projects I maintain: I use a tool that automatically generates release notes whenever I publish a new version and this tool uses the title of the issues to populate the release notes (you can see release notes here). Therefore it's crucial to have an issue for each PR in order for the change/fix/new feature to be mentioned in the release notes.

mxswd commented 1 year ago

Thanks for this PR. I made some recommendations.

Also make sure to raise an issue. This is particularly important in this case since you are introducing breaking changes. We must document these changes and ensure developers are aware of what they will need to change in their code.

I can move the argument to the end of the parameter list and it shouldn't be a breaking change. What are your thoughts on that?

Jericho commented 1 year ago

I can move the argument to the end of the parameter list and it shouldn't be a breaking change. What are your thoughts on that?

Changing a public method's signature is a breaking change no matter the position of the new parameter. It's less problematic when the new parameter is optional and added at the end of the parameter list, as you suggested, but it's still a breaking change. Developers still need to recompile their code with the updated reference, they can't simply copy the updated assembly into the folder containing their binaries.

Also, you'll notice that the final parameter of all async methods in ZoomNet is the CancellationToken. So, for consistency sake, I wouldn't want to change that.

Plus, I like that you added this new parameter right next to the password parameter. Makes it more obvious (to me at least) that the two parameters are kinda related.

I'm comfortable with marking the issue you recently created as a "Breaking Change" and adding comment in the release notes to explain the exception developers are likely to get in Visual Studio and what change they need to make to resolve it. I volunteer to write this comment. I'll present it to you for feedback prior to merging the PR.