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.

Sign up for GitHub

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

Jump to bottom

feat(config): validate config after load_config #4381

Merged
merged 5 commits into from
Jul 10, 2020
Merged

feat(config): validate config after load_config #4381

merged 5 commits into from
Jul 10, 2020

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Jun 26, 2020

What does it do?

Continuation fo #4371 and supersedes #4376.

How to test

git clone -b config-validation https://github.com/sukkaw/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@SukkaW SukkaW requested review from curbengh and a team June 26, 2020 10:16
@coveralls
Copy link

coveralls commented Jun 26, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 97.812% when pulling c2d3428 on SukkaW:config-validation into 65961d3 on hexojs:master.

@SukkaW SukkaW requested review from curbengh and a team June 27, 2020 04:49
@SukkaW SukkaW mentioned this pull request Jun 27, 2020
Closed
2 tasks
@curbengh
Copy link
Contributor

curbengh commented Jun 28, 2020 •
edited
Loading

Is it possible to run it in generate & server only? Currently this is also executed in clean.

Created #4386 for hexo clean. Tested combining #4386 and this PR, and it's working.

curbengh
curbengh requested changes Jun 28, 2020
View reviewed changes
lib/hexo/validate_config.js Outdated Show resolved Hide resolved
lib/hexo/validate_config.js Outdated Show resolved Hide resolved
@curbengh curbengh mentioned this pull request Jun 28, 2020
Merged
2 tasks
@curbengh curbengh added this to the 5.0.0 milestone Jun 28, 2020
@jiangtj
Copy link
Member

jiangtj commented Jun 29, 2020 •
edited
Loading

How about use some schema validator? e.g. ajv joi etc.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 29, 2020

@jiangtj It can be a part of a roadmap. But so far we just don't need it.

@SukkaW SukkaW requested a review from curbengh June 30, 2020 03:16
curbengh
curbengh reviewed Jun 30, 2020
View reviewed changes
lib/hexo/validate_config.js
Comment on lines +12 to +14
if (config.url.trim().length <= 0) {
throw new TypeError('Invalid config detected: "url" should not be empty!');
}
Copy link
Contributor

@curbengh curbengh Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

suggest to include undefined checking here as well (and root), need to run before 'string' checking.

Copy link
Member Author

@SukkaW SukkaW Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@curbengh Already added in L9:

  if (typeof config.url !== 'string') {

undefined just won't pass it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That check is inaccurate for undefined since it's an empty value.

Copy link
Member Author

Choose a reason for hiding this comment

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

@curbengh I have already added an undefined test cases. It seems working properly.

@SukkaW SukkaW requested a review from curbengh June 30, 2020 15:03
curbengh
curbengh reviewed Jul 2, 2020
View reviewed changes
test/scripts/hexo/validate_config.js Outdated
should.fail();
} catch (e) {
e.name.should.eql('TypeError');
e.message.should.eql('Invalid config detected: "url" should be string!');
Copy link
Contributor

Choose a reason for hiding this comment

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

undefined is practically an empty value. If a user accidentally removes the value of url: or even the key itself, it's clearer to say "url" should not be empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although for YAML, undefined is usually caused by empty value. But Hexo also supports config in json format (Although most people just won't using json).

Anyway, I will update the error message then.

@SukkaW SukkaW requested a review from curbengh July 2, 2020 07:36
@stevenjoezhang stevenjoezhang mentioned this pull request Jul 2, 2020
Closed
13 tasks
@SukkaW
Copy link
Member Author

SukkaW commented Jul 6, 2020

@curbengh Would you mind have a look at the PR again?

@SukkaW SukkaW requested a review from a team July 8, 2020 10:17
curbengh
curbengh approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@curbengh curbengh left a comment

Choose a reason for hiding this comment

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

  if (typeof config.url === 'undefined' || config.url === null || config.url.trim().length <= 0) {
    throw new TypeError('Invalid config detected: "url" should not be empty!');
  }

I prefer the above, but this PR is acceptable.

@SukkaW SukkaW merged commit cb4a6f3 into hexojs:master Jul 10, 2020
@NoahDragon NoahDragon mentioned this pull request Aug 26, 2020
Open
53 tasks
@stevenjoezhang stevenjoezhang mentioned this pull request Nov 20, 2020
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@curbengh curbengh curbengh approved these changes

@2997ms 2997ms 2997ms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@SukkaW @coveralls @curbengh @jiangtj @2997ms