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

[Downloader] Catch OSErrors from invalid repo names #3029

Merged
merged 7 commits into from Oct 5, 2019

Conversation

@Flame442
Copy link
Member

commented Oct 4, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

Prevents repo names from containing anything except for a-z, 0-9, and underscores.
This fix might need to happen at a lower level, this PR only addresses the command [p]repo add

Catches OSErrors from invalid repo names, pretty much only on Windows. Also recommends users only use safe characters in the docstring.
Prevents repo names from containing anything except for a-z, 0-9, underscores, and hyphens.

Fixes #2999, Fixes #2827

@Flame442 Flame442 requested a review from tekulvw as a code owner Oct 4, 2019
@Flame442 Flame442 requested a review from Twentysix26 as a code owner Oct 4, 2019
@Flame442 Flame442 added the Type: Fix label Oct 4, 2019
@jack1142

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Technically this fixes the problem though when you're already doing something about this, I wonder if we could also add hyphens to set of allowed characters, especially that some repos already use those in repo names and it would seem logical to allow such names.

@mikeshardmind

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

I'm somewhat concerned that this is an overly restrictive set and that it would be better to detect when a file path is problematic to the host system and inform users rather than error out, and adjust the help text to match that, however I also see how this is not the easiset approach.

(On windows) There are other things which are invalid which that regex allows

AUX
NUL
CON
COM[1-9]

And rather than handling each possible failure case preventatively, it is likely more productive to just detect the error and ask the user to use something their filesystem and OS allow, and adjust the help text to match that.

@Flame442 Flame442 changed the title [Downloader] Ensure repo names only contain the characters stated [Downloader] Catch OSErrors from invalid repo names Oct 4, 2019
Flame442 added 4 commits Oct 4, 2019
@mikeshardmind mikeshardmind merged commit 9b60816 into Cog-Creators:V3/develop Oct 5, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@mikeshardmind mikeshardmind added this to the 3.2.0 milestone Oct 5, 2019
@Flame442 Flame442 deleted the Flame442:patch-1 branch Oct 5, 2019
AnonGuy added a commit to AnonGuy/Red-DiscordBot that referenced this pull request Oct 8, 2019
* [Downloader] Ensure repo names only contain the characters stated

* Create 2827.bugfix.rst

* [Downloader] Catch OSErrors from invalid filenames

* Update 2827.bugfix.rst

* Style

* do the thing again

* Update 2827.bugfix.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.