-
-
Notifications
You must be signed in to change notification settings - Fork 305
feat: support short form teams url #1869
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
Conversation
Summary of ChangesHello @Yumeo0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial update to the application's URL parsing logic, enabling it to correctly recognize and handle the recently introduced shorter URL format for Microsoft Teams meetings. This enhancement ensures seamless integration and improved user experience when interacting with Teams meeting links, adapting to changes in the platform's URL structure. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully adds support for the shorter Microsoft Teams URL format by updating the validation regex. The version is bumped to 2.5.14, and the release notes are updated accordingly. The changes are correct and achieve the intended goal. I have one suggestion to improve code style and performance by defining the regular expression as a constant outside the function to avoid recompilation on each call.
|
Hi @Yumeo0 , thanks for the commit. I think we will need to also modify the urls in the config location, otherwise it will not know it has to open the url (I might be wrong and it getting translated). If you can also run |
|
@IsmaelMartinez I updated the config to also accept the shortform urls and used that regex instead of the local variable I had before. I also split the config up into "all teams links" and "meeting urls only". |
|
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hard work. Just re-use the meetupJoinRegEx instead of creating a new one, as otherwise the users that are overwriting this value with the config option will get a change of behaviour (it will stop overwriting the right one). I put some hopefully easy to accept suggestions (that should work but haven't tested). Thanks again!
| default: "^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|meet)", | ||
| describe: "Regex for Teams meetup-join links only", | ||
| type: "string", | ||
| }, | ||
|
|
||
| allTeamsLinksRegEx: { | ||
| default: "^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|channel|chat|meet)", | ||
| describe: "Regex for all Teams-related links (meetup, channel, chat, meet)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies the confusion but just re-use the same RegEx, otherwise people overwriting this local configuration in their config files will see a change in behaviour.
| default: "^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|meet)", | |
| describe: "Regex for Teams meetup-join links only", | |
| type: "string", | |
| }, | |
| allTeamsLinksRegEx: { | |
| default: "^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|channel|chat|meet)", | |
| describe: "Regex for all Teams-related links (meetup, channel, chat, meet)", | |
| default: "^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|channel|chat|meet)", | |
| describe: "Regex for all Teams-related links (meetup, channel, chat, meet)", |
| for (const arg of args) { | ||
| console.debug( | ||
| `testing RegExp processArgs ${new RegExp(config.meetupJoinRegEx).test( | ||
| `testing RegExp processArgs ${new RegExp(config.allTeamsLinksRegEx).test( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `testing RegExp processArgs ${new RegExp(config.allTeamsLinksRegEx).test( | |
| `testing RegExp processArgs ${new RegExp(config.meetupJoinRegEx).test( |
| )}` | ||
| ); | ||
| if (new RegExp(config.meetupJoinRegEx).test(arg)) { | ||
| if (new RegExp(config.allTeamsLinksRegEx).test(arg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (new RegExp(config.allTeamsLinksRegEx).test(arg)) { | |
| if (new RegExp(config.meetupJoinRegEx).test(arg)) { |
| | `spellCheckerLanguages` | `array` | `[]` | Array of languages to use with Electron's spell checker | | ||
| | `logConfig` | `object` | `{ transports: { console: { level: "info" }, file: { level: false } } }` | Electron-log configuration | | ||
| | `meetupJoinRegEx` | `string` | `^https://teams.(microsoft\|live).com/.*(?:meetup-join\|channel\|chat)` | Meetup-join and channel regular expression | | ||
| | `allTeamsLinksRegEx` | `string` | `^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|channel|chat|meet)` | All Teams-related links regular expression | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `allTeamsLinksRegEx` | `string` | `^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|channel|chat|meet)` | All Teams-related links regular expression | | |
| | `meetupJoinRegEx` | `string` | `^https://teams\\.(microsoft|live)\\.com/.*/(?:meetup-join|channel|chat|meet)` | All Teams-related links regular expression | |
|
done the changes in #1915 as I don't have permission to touch your repo. I did pull your commits so you should get credit for your work. Thanks again for your contribution! |
I kind of forgot to do this as I had a bit of stress the last couple of weeks. Thanks for your work ❤️ |
|
no problem and thanks for your work/help! |



changes the url regex pattern to support the shorter teams url format