-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
All: Seperate allPossibleCategories to @joplin/lib #6754
Conversation
packages/generator-joplin/generators/app/templates/webpack.config.js
Outdated
Show resolved
Hide resolved
packages/generator-joplin/generators/app/templates/package_TEMPLATE.json
Outdated
Show resolved
Hide resolved
Ok I think it's good now. If you could fix the conflict we can merge. |
I‘ve fixed the conflict now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the below comment, this looks good to me!
@@ -73,7 +73,7 @@ function validateCategories(categories) { | |||
if (!categories) return null; | |||
if ((categories.length !== new Set(categories).size)) throw new Error('Repeated categories are not allowed'); | |||
categories.forEach(category => { | |||
if (!allPossibleCategories.includes(category)) throw new Error(`${category} is not a valid category. Please make sure that the category name is lowercase. Valid Categories are: \n${allPossibleCategories}\n`); | |||
if (!allPossibleCategories.map(category => { return category.name; }).includes(category)) throw new Error(`${category} is not a valid category. Please make sure that the category name is lowercase. Valid Categories are: \n${allPossibleCategories}\n`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!allPossibleCategories.map(category => { return category.name; }).includes(category)) throw new Error(`${category} is not a valid category. Please make sure that the category name is lowercase. Valid Categories are: \n${allPossibleCategories}\n`); | |
if (!allPossibleCategories.map(category => { return category.name; }).includes(category)) throw new Error(`${category} is not a valid category. Please make sure that the category name is lowercase. Valid categories are: \n${allPossibleCategories}\n`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also ${category.name} is not a valid category
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed these typo now.
Any update here @Retr0ve? There's not much code but so many mistakes |
827c220
to
3d3b870
Compare
Thx for checking. I've updated the code. |
@@ -73,7 +73,7 @@ function validateCategories(categories) { | |||
if (!categories) return null; | |||
if ((categories.length !== new Set(categories).size)) throw new Error('Repeated categories are not allowed'); | |||
categories.forEach(category => { | |||
if (!allPossibleCategories.includes(category)) throw new Error(`${category} is not a valid category. Please make sure that the category name is lowercase. Valid Categories are: \n${allPossibleCategories}\n`); | |||
if (!allPossibleCategories.map(category => { return category.name; }).includes(category)) throw new Error(`${category.name} is not a valid category. Please make sure that the category name is lowercase. Valid categories are: \n${allPossibleCategories}\n`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error(`${category.name}
In fact it should be "category" here, not "category.name", isn't it? It doesn't look like you have tested or even read your code at all. I made a mistake by suggesting this change, but you need to be responsible for what you're doing and not rely on reviewers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested and fixed now
To better maintain the plugin categories, I separate allPossibleCategories variable from webpack script to a json file under @joplin/lib.