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

Auto generated project name includes numerics #1254

Merged
merged 6 commits into from
Sep 15, 2021

Conversation

Scowley4
Copy link
Contributor

@Scowley4 Scowley4 commented Sep 14, 2021

This change would allow numerics to be included in project names.

Checklist

  • I have confirmed that my PR passes all linting checks
  • I have included test cases
  • I have updated the documentation
  • I have added an entry to the changelog

@Scowley4
Copy link
Contributor Author

Scowley4 commented Sep 14, 2021

I guess now that I'm looking a little more, is the isalpha meant to prevent projects that start with a numeric? As shown:

sys.modules[f"brownie.project.{name}"] = self # type: ignore


Python imports can by any identifier:

(letter|"_") (letter | digit | "_")*


If the changed line is protecting for python purposes, maybe throw an error if the project name actually starts with a numeric. I'm just thinking some projects use versioning in their smart contracts. For example: UniswapV2 vs UniswapV3.

@iamdefinitelyahuman
Copy link
Member

Yeah, the check could definitely be modified so that isalpha is only applied to the first digit.

@Scowley4 Scowley4 closed this Sep 15, 2021
@Scowley4 Scowley4 deleted the patch-1 branch September 15, 2021 06:40
@Scowley4 Scowley4 restored the patch-1 branch September 15, 2021 06:52
@Scowley4
Copy link
Contributor Author

Sorry, didn't realize renaming branch would mess up the PR :/

I've added an exception and written a check for the first character of a project/package name, throwing BadProjectName if the character isn't alphabetic.

@Scowley4 Scowley4 reopened this Sep 15, 2021
@iamdefinitelyahuman iamdefinitelyahuman merged commit f0e6747 into eth-brownie:master Sep 15, 2021
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