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

Revise warning message for configuration "pages" property #1318

Merged
merged 4 commits into from
Aug 4, 2023

Conversation

mint-thompson
Copy link
Collaborator

Fixes #1308 and completes task CIMPL-1144.

An unrecognized property within the "pages" property may or may not represent a subpage. Revise warning message to better acknowledge both possibilities.

An unrecognized property within the "pages" property may or may not
represent a subpage. Revise warning message to better acknowledge both
possibilities.
cmoesel
cmoesel previously approved these changes Aug 3, 2023
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

The new messages provide more guidance and allow for more of the possible reasons a user might get that error. Well done!

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

Just had one small comment/conversation with myself. Let me know what you think.

Comment on lines 922 to 924
let recommendation = `Check that ${
singular ? 'this property is' : 'these properties are'
} spelled and indented correctly.`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the issue that brought this up, I'm wondering if this recommendation message should say "spelled, capitalized, and indented correctly"? It is definitely wordy and clunky, but I think if I had written Title how it is in the issue, I might still be confused because there are no typos there. But, I can be convinced this falls under spelling, so if you really don't like this, no worries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, is there a reason we are case sensitive here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Properties in JS/TS are normally case-sensitive. I'm okay with sticking with case-sensitive properties for the configuration, so I'll add in a note about capitalization. We could potentially try to make configuration processing allow for arbitrary capitalization for the expected properties, but I think that would fall outside the scope of this PR.

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

Still good!

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

I love an Oxford comma, so this is great. Thanks!

@mint-thompson mint-thompson merged commit e17a19f into master Aug 4, 2023
14 checks passed
@mint-thompson mint-thompson deleted the cimpl-1144-config-warning-pages branch August 4, 2023 18:23
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.

Warning messages about unrecognized configuration properties under "pages" always suggest file type endings
3 participants