-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Update build scripts to use improved settings #286
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #286 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 73 73
Lines 7473 8447 +974
==========================================
+ Hits 7473 8447 +974
Continue to review full report at Codecov.
|
@@ -42,15 +42,15 @@ | |||
* @copyright 2015 Titus Wormer | |||
* @license MIT | |||
* @example | |||
* {"name": "ok.md", "setting": 4} | |||
* {"name": "ok.md", "config": 4} |
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 noticed that we currently use config
in all the readmes:
remark-lint/script/build-rules.js
Line 540 in 014fca7
value: 'unified().use(' + camelcased + '[, config])' |
What do you think about replacing
setting
with that?
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.
yeah sure!
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.
Thanks!
@@ -157,16 +157,16 @@ Due to this, it’s recommended to configure this rule with `2`. | |||
|
|||
##### `ok.md` | |||
|
|||
When configured with `2`. | |||
When configured with `4`. |
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 also wrapped {config}
in an object. It's the tests
key, which changes the order of some examples, because the tests
JS object isn't order preserving: #279 (comment)
Before some examples were in numeric order, now they're in the order defined in the docblocks. Before the key was '4'
, now it's '{"config":4}'
.
This is also a cosmetic change --- it extracts the change in example order from #279 (review). That PR changes the tests
key from {config}
-> {settings, config}
.
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.
Am I correct in seeing this PR as doing two things only (a. settings -> config, b. config -> {config}) and nothing else?
Or do I need to properly read everything to see if there’s something weird somewhere? 😅
I fixed the errors on |
That's correct:
Awesome thanks! 🙌 I'll remove the |
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.
okay then!
This comment has been minimized.
This comment has been minimized.
Thanks! :) |
Initial checklist
Description of changes
I'm in favor of minimizing the #279 (review) diffs, I'm also in favor of disambiguating
setting
andsettings
. This PR extracts cosmetic changes from #279 (comment) --- I propose merging this one and then rebasing #279, to make it smaller?