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

Check that most generator <name> positional arguments don't start with non-word characters #7143

Merged
merged 5 commits into from
Dec 14, 2022

Conversation

cannikin
Copy link
Member

Closes #7136

Copy link
Collaborator

@pantheredeye pantheredeye left a comment

Choose a reason for hiding this comment

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

Looks good - I tested generating / and /foo and both break with the same issue. I tested using \ and \foo and it works fine... Would \ cause any issues on linux?

Copy link
Collaborator

@pantheredeye pantheredeye left a comment

Choose a reason for hiding this comment

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

Reviewing is new to me... so just to let you know - i added some comments in the code section where I tested using . and , and it messed up the routes file. May be worth validating against these characters, also. I have to go now but may test some more random characters later.

@cannikin
Copy link
Member Author

You've got the soul of a real QA person! :) I'll negate the check: instead of checking that the first character isn't from a list of weird ones, I'll check that first character must be a letter or number.

Copy link
Collaborator

@pantheredeye pantheredeye left a comment

Choose a reason for hiding this comment

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

added single comment; looks good. I have not compiled and tested.

Co-authored-by: Barrett Burnworth <77902178+pantheredeye@users.noreply.github.com>
@cannikin cannikin added the fixture-ok Override the test project fixture check label Dec 14, 2022
@cannikin cannikin changed the title Check that most generator <name> positional arguments don't start with / Check that most generator <name> positional arguments don't start with non-word characters Dec 14, 2022
@cannikin cannikin merged commit 5895ae3 into main Dec 14, 2022
@cannikin cannikin deleted the rc-generate-validate branch December 14, 2022 22:58
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Dec 14, 2022
github-actions bot pushed a commit that referenced this pull request Dec 14, 2022
…h / (#7143)

* Check that the <name> positional argument doesn't start with /

* Add tests

* Update existing test to include a name

* Negate check to make sure names must start with a character (letters, numbers or underscore)

* Update packages/cli/src/commands/generate/helpers.js

Co-authored-by: Barrett Burnworth <77902178+pantheredeye@users.noreply.github.com>

Co-authored-by: Barrett Burnworth <77902178+pantheredeye@users.noreply.github.com>
jtoar pushed a commit that referenced this pull request Dec 15, 2022
…h / (#7143)

* Check that the <name> positional argument doesn't start with /

* Add tests

* Update existing test to include a name

* Negate check to make sure names must start with a character (letters, numbers or underscore)

* Update packages/cli/src/commands/generate/helpers.js

Co-authored-by: Barrett Burnworth <77902178+pantheredeye@users.noreply.github.com>

Co-authored-by: Barrett Burnworth <77902178+pantheredeye@users.noreply.github.com>
@jtoar jtoar removed this from the next-release-patch milestone Dec 22, 2022
@jtoar jtoar added this to the v3.8.0 milestone Dec 22, 2022
})
it('does nothing if name is valid', () => {
expect(() => helpers.validateName('foo')).not.toThrow()
expect(() => helpers.validateName('foo/')).not.toThrow()
expect(() => helpers.validateName('_foo')).not.toThrow()
expect(() => helpers.validateName('1foo')).not.toThrow()
expect(() => helpers.validateName('foo/bar')).not.toThrow()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cannikin Thinking about the problem with creating a page in a sub directory. I was checking to update the regex pattern in this PR, but this line tests for a directory/page and expects no error. 🤷‍♂️ It seems like this would pass the subdirectory creation pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

So there's no actual error during the generation, but the page itself, when React tries to render it, throws an error because the route function call is invalid syntax routes.admin / dashboard().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:fix This PR is a fix topic/generators-&-scaffolds
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug?]: Generator fails if [name] positional argument is '/'
3 participants