Skip to content

Update form.py#127

Closed
IlluminatiFish wants to merge 4 commits into
python-discord:mainfrom
IlluminatiFish:main
Closed

Update form.py#127
IlluminatiFish wants to merge 4 commits into
python-discord:mainfrom
IlluminatiFish:main

Conversation

@IlluminatiFish
Copy link
Copy Markdown

@IlluminatiFish IlluminatiFish commented Jan 4, 2022

A regex check to validate webhook urls instead of a substring check

Copy link
Copy Markdown
Contributor

@HassanAbouelela HassanAbouelela left a comment

Choose a reason for hiding this comment

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

Hi,

Please follow the proper contribution guidelines before making a PR to open source projects.

Is there a reason for this change? Running a regex over this field seems kind of pointless.

@IlluminatiFish
Copy link
Copy Markdown
Author

Hi,

Please follow the proper contribution guidelines before making a PR to open source projects.

Is there a reason for this change? Running a regex over this field seems kind of pointless.

To be frankly honest the contributing.md was hidden in the .github folder, my bad for not looking for it more in depth.
I think running a regex over the field would be more beneficial as it is more lenient on the webhook url format

@HassanAbouelela
Copy link
Copy Markdown
Contributor

The contributing guidelines are placed in the recommended folder, as per github's instructions, to ensure they show up in the side bar or when you try to open a PR.

As for using the regex here, I don't think it makes sense to add. The only benefit it has over the current implementation is that it enables the discordapp domain, and enables the specification of API version, neither of which you'll get unless you try to type the URL manually. If you copy the URL from inside the app, it'll be of the form [canary|ptb].discord.com/api/webhook, which will work with the current implementation. The disadvantages to this regex is that it's very hard to read (especially because you didn't use a raw string), and it's less performant than the text lookup.

@IlluminatiFish
Copy link
Copy Markdown
Author

The contributing guidelines are placed in the recommended folder, as per github's instructions, to ensure they show up in the side bar or when you try to open a PR.

As for using the regex here, I don't think it makes sense to add. The only benefit it has over the current implementation is that it enables the discordapp domain, and enables the specification of API version, neither of which you'll get unless you try to type the URL manually. If you copy the URL from inside the app, it'll be of the form [canary|ptb].discord.com/api/webhook, which will work with the current implementation. The disadvantages to this regex is that it's very hard to read (especially because you didn't use a raw string), and it's less performant than the text lookup.

Will keep that noted for next time when looking for the contributing guidelines.
Fair enough, i guess we can close this out.

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.

2 participants