Disable heading IDs by joshbruce · Pull Request #1190 · markedjs/marked

joshbruce · 2018-03-31T18:20:33Z

Marked version: 0.3.19

Markdown flavor: n/a

Description

PR adds functionality to disable header IDs.

This is also related to work being performed on #1160.

For some reason testing options does not work in isolation.

Contributor

Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
no tests required for this PR.
If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

Draft GitHub release notes have been updated.
CI is green (no forced merge required).
Merge PR

q

styfle · 2018-04-01T23:07:49Z

You should also add this to the documentation so users are aware of this feature.

UziTech · 2018-04-02T01:26:36Z

You should always create new branches from master to avoid including changes from other PRs

UziTech

This PR should only contain changes to lib/marked.js

joshbruce · 2018-04-02T11:43:34Z

@styfle: Updated documentation. Maybe I should make that a separate PR given the review from @UziTech.

@UziTech: Believe it or not, I actually did branch off from master; I just did the same --save-dev from the other PR. Our test harness uses min; therefore, to get running tests I'm pretty sure there has to be updates to that file as well. Will put together some other PRs I think might cover the comments...sorry for the multiples and possible closings.

joshbruce · 2018-04-02T12:05:16Z

See #1192 and #1193

UziTech

Sorry, I saw the integration tests and assumed you had included changes from #1160

I was thinking integration tests would be more for testing specs and making sure marked works in different environments. I think this should just require a unit test on the renderer.

joshbruce · 2018-04-02T17:06:22Z

@UziTech: Think I follow what you're asking, though not sure if I could do it easily. I'm adding this functionality to test the headers for CommonMark as none of the examples use IDs; so, the capability will be in the integration test suite at some point.

Will see what I can do on the more unit-test side of the house.

UziTech · 2018-04-02T18:04:35Z

Ya that is fine if this option is used in the integration tests, but there is no need to have an integration test specifically for this feature.

The way I see integration tests is if a feature needs to test multiple parts of the software (and possibly the platform) than it should have an integration test (e.g. spec tests need to test the whole application; lexing, parsing, and rendering)

This feature only affects the rendering so it could be a unit test something like:

it('should add header ids by default',function(){varrederer=newmarked.Renderer(marked.defaults);varheader=rederer.heading('test',1,'test');expect(header).toBe('<h1 id="test">test</h1>\n');});it('should ignore header ids',function(){varrederer=newmarked.Renderer({headerIds: false});varheader=rederer.heading('test',1,'test');expect(header).toBe('<h1>test</h1>\n');});

…into optional-heading-ids

joshbruce · 2018-04-02T19:24:09Z

I believe we've covered all the comments and a little beyond.

Uses unit tests instead of integration tests.
Documentation updated (including some things that were missing and ordering them alphabetically in the code and the docs).
Contributor checklist updated.

If we merge this, I will close #1192 and #1193. Then I should be able to go back to #1160 to handle the failing header things, which should then only have things that are actually broken remaining.

UziTech · 2018-04-02T19:31:18Z

 "glob-to-regexp": "0.3.0",
+ "html-differ": "^1.3.4",
 "jasmine": "^3.1.0",
+ "jasmine2-custom-message": "^0.9.0",


these are not needed for this PR

UziTech · 2018-04-02T19:40:32Z

+
+describe('Test heading ID functionality', function() {
+	it('should add id attribute by default', function() {
+ var renderer = new sut.Renderer(sut.defaults);


Could we change sut to something like markedUnderTest or something like that? It feels very unintuitive that these defaults are coming from marked.
I feel like the only reason to distinguish the "system under test" is if we would need the same system not under test. (e.g. Jasmine testing Jasmine with Jasmine) I can't think of a reason we would ever need that.

UziTech · 2018-04-02T19:42:47Z

 var marked = require('../../marked.min.js');
+var HtmlDiffer = require('html-differ').HtmlDiffer,
+ htmlDiffer = new HtmlDiffer();
+var since = require('jasmine2-custom-message');


These are not needed

UziTech · 2018-04-02T19:44:05Z

 breaks: false,
+ gfm: true,
+ headerPrefix: '',
+ headerIds: true,


'I' comes before 'P'

Good catch...funny enough, I got it right in the table. :)

UziTech

You should remove the package-lock.json changes.

Are we supposed to be including marked.min.js changes in PRs? or is that supposed to wait for a release?

#1192 seems to duplicate a lot of these changes. Should we do those changes in this PR?

joshbruce · 2018-04-03T14:20:20Z

Removed the package-lock file entirely.
I had to do it because initially our test harness worked off min. We've also done PR merges with it changed in the past. I don't think it necessarily has to wait for a release...keeps us with the min reflects master mentality.
Yes. If we approve this I will close Alphabetize options & update table docs #1192 and Add libraries and modify integration tests #1193 per comment: Disable heading IDs #1190 (comment)

styfle · 2018-04-03T15:31:18Z

- "eslint": "^4.15.0",
- "eslint-config-standard": "^11.0.0-beta.0",
- "eslint-plugin-import": "^2.8.0",
+ "eslint": "^4.19.1",


Why are you updating dev dependencies? This should be in a different PR in my opinion.

UziTech · 2018-04-03T15:31:53Z

- "showdown": "*",
- "uglify-js": "^3.3.10"
+ "markdown-it": "^8.4.1",
+ "showdown": "^1.8.6",


Why are these updated?

Not intentional.
Will try to reset manually.

styfle · 2018-04-03T18:42:59Z

- "version": "5.4.1",
- "resolved": "https://registry.npmjs.org/acorn/-/acorn-5.4.1.tgz",
- "integrity": "sha512-XLmq3H/BVvW6/GbxKryGxWORz1ebilSsUDlyC27bXhWGWAZWkGwS6FLHjOlwFXNFoWFQEO/Df4u0YYd0K3BQgQ==",
+ "version": "5.5.3",


Can you revert this file too?

* Add option to disable heading ids * Alphabetize and add options to docs

Cannot set options
8b60260
q

joshbruce requested review from UziTech and styfle March 31, 2018 18:20

Updated min
3d7788b

UziTech requested changes Apr 2, 2018
View reviewed changes

Alphabetize and add options to docs
e4f2f81

UziTech reviewed Apr 2, 2018
View reviewed changes

Josh Bruceand others added 4 commits April 2, 2018 15:13

Make unit tests
83806c0

Merge branch 'master' into optional-heading-ids
c57dff5

Fix lint and remove intergration test
abfe340

Merge branch 'optional-heading-ids' ofhttps://github.com/8fold/marked…
84a3c29
…into optional-heading-ids

UziTech requested changes Apr 2, 2018
View reviewed changes

Post review
a67c8f6

styfle approved these changes Apr 2, 2018
View reviewed changes

revert integration test file
12976eb

UziTech requested changes Apr 3, 2018
View reviewed changes

Update package
7872103

Refresh package-lock
cc0d396

styfle requested changes Apr 3, 2018
View reviewed changes

UziTech requested changes Apr 3, 2018
View reviewed changes

Trying to roll back package.json
7c87d53

styfle reviewed Apr 3, 2018
View reviewed changes

zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
Disable heading IDs (markedjs#1190)
b055f87
* Add option to disable heading ids * Alphabetize and add options to docs

snyk-bot mentioned this pull request Dec 29, 2022
[Snyk] Security upgrade marked from 0.3.5 to 0.4.0 Omrisnyk/goof-jenkins#22
Open

Uh oh!

Conversation

joshbruce commented Mar 31, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor

Committer

Uh oh!

styfle commented Apr 1, 2018

Uh oh!

UziTech commented Apr 2, 2018

Uh oh!

UziTech left a comment

Choose a reason for hiding this comment

Uh oh!

joshbruce commented Apr 2, 2018

Uh oh!

joshbruce commented Apr 2, 2018

Uh oh!

UziTech left a comment

Choose a reason for hiding this comment

Uh oh!

joshbruce commented Apr 2, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UziTech commented Apr 2, 2018

Uh oh!

joshbruce commented Apr 2, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UziTech left a comment

Choose a reason for hiding this comment

Uh oh!

joshbruce commented Apr 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joshbruce commented Mar 31, 2018•
edited
Loading

joshbruce commented Apr 2, 2018•
edited
Loading

joshbruce commented Apr 2, 2018•
edited
Loading

joshbruce commented Apr 3, 2018•
edited
Loading