Skip to content

chore: Reduce explicit usage of new lines#338

Merged
AndrewSouthpaw merged 44 commits into
thlorenz:masterfrom
thompson-tomo:patch-1
Apr 21, 2026
Merged

chore: Reduce explicit usage of new lines#338
AndrewSouthpaw merged 44 commits into
thlorenz:masterfrom
thompson-tomo:patch-1

Conversation

@thompson-tomo

Copy link
Copy Markdown
Contributor

This uses an array of toc contents which is only combined into a string at the end which reduces the explicit usage of EOL characters and increases readability.

@AndrewSouthpaw

Copy link
Copy Markdown
Collaborator

@thompson-tomo Some new merge conflicts on this, unfortunately.

@thompson-tomo

Copy link
Copy Markdown
Contributor Author

All fixed. @AndrewSouthpaw & would be good to get your thoughts on #339 which is the successor to this.

@AndrewSouthpaw AndrewSouthpaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a couple small nits

Comment thread lib/transform.js Outdated
Comment thread lib/transform.js Outdated
@thompson-tomo

Copy link
Copy Markdown
Contributor Author

@AndrewSouthpaw feedback implemented

@AndrewSouthpaw

Copy link
Copy Markdown
Collaborator

Checks are failing btw

@thompson-tomo

Copy link
Copy Markdown
Contributor Author

Sorry about that @AndrewSouthpaw fixed now.

Comment thread test/transform.js
Comment on lines +12 to +13
// build the expected content
var pragma = contentGenerator.pragmaMarkers(syntax || 'md');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm I'd rather not build the eol based on the implementation output. It's not exactly tautological (because of a detail in the test assertion), but it's close.

If we want to eventually test eol configs for both \n and \r\n etc., I'd rather create a test matrix or write them out explicitly.

This will also allow us to drop the eol being exposed from transform, which is currently only used by the test.

Thoughts?

@thompson-tomo thompson-tomo Apr 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree it is not great but couldn't think of a better way as i wanted to avoid adding another argument to the method but will go down that path. Please take another look as I have made the change & have also updated #339

@thompson-tomo

Copy link
Copy Markdown
Contributor Author

@AndrewSouthpaw i think this is good to merge now.

@AndrewSouthpaw AndrewSouthpaw merged commit e1ba3d5 into thlorenz:master Apr 21, 2026
5 checks passed
AndrewSouthpaw pushed a commit that referenced this pull request Apr 26, 2026
Closes: #161 
Closes: #216

Tool now detects what line endings have been used in the content and uses that in the generated toc to strive for consistency.

Blocked by: #337 & #338
@thompson-tomo thompson-tomo deleted the patch-1 branch April 26, 2026 01:35
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.

2 participants