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

[legacy-framework] show an error if model name is invalid in blitz generate #2802

Merged
merged 10 commits into from
Oct 11, 2021
Merged

[legacy-framework] show an error if model name is invalid in blitz generate #2802

merged 10 commits into from
Oct 11, 2021

Conversation

martinsaxa
Copy link

Closes: blitz-js/legacy-framework#238

What are the changes and their implications?

Blitz-cli generate options will check if model name contains :. If it does, it will throw an error. Usually this happens when someone forget to pass model name e.g. blitz generate all name:string

Bug Checklist

  • Integration test added (see test docs if needed)

Feature Checklist

Copy link
Contributor

@beerose beerose left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Instead of using console.warn, we should use Blitz's log function log.error(...). It's already imported into this file.

Apart from that, we should check if the model name contains only valid characters instead of checking for particular invalid symbols, e.g. : inside of the model name is only one of the invalid characters. Model names should satisfy the following expression: [A-Za-z][A-Za-z0-9_]*.

@martinsaxa
Copy link
Author

Thanks for review, I will look into these issues.

Copy link
Contributor

@beerose beerose left a comment

Choose a reason for hiding this comment

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

I'm sorry for complicating things, but we just merged a similar PR (#2813), which was also handling model names, but it only checks for reserved words.

However, there was a new function validateModelName added, and I'm wondering if we can combine your changes into this function? What do you think about adding the check to the validateModelName and throwing an error in a similar way?

@martinsaxa
Copy link
Author

martinsaxa commented Oct 8, 2021

I just looked at that PR and function and I thing you have an great idea. I'm going to work on that
this weekend.

Co-authored-by: Aleksandra Sikora <[email protected]>
beerose
beerose previously approved these changes Oct 11, 2021
Copy link
Contributor

@beerose beerose left a comment

Choose a reason for hiding this comment

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

Awesome, thanks again!

beerose
beerose previously approved these changes Oct 11, 2021
@beerose beerose changed the title add err when model name not passed to cli generate show an error if model name is invalid in blitz generate Oct 11, 2021
@beerose beerose merged commit 48f762c into blitz-js:canary Oct 11, 2021
@itsdillon itsdillon changed the title show an error if model name is invalid in blitz generate [legacy-framework] show an error if model name is invalid in blitz generate Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blitz generate should check if model name is passed correctly
3 participants