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

Updated help output and removed extra logs from stdOut option #112

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Updated help output and removed extra logs from stdOut option #112

wants to merge 3 commits into from

Conversation

RichardLitt
Copy link
Contributor

I reorganized, limited to 80 chars, added --stdout, added shortcut options, put a space between the path description to make it more readable, and also fixed ambiguous working in the stdOut function.

@RichardLitt
Updated help output
f76b8a6 
I reorganized, limited to 80 chars, added `--stdout`, added shortcut options, put a space between the path description to make it more readable, and also fixed ambiguous working in the stdOut function, as well as added periods.
@RichardLitt
Copy link
Contributor Author

@thlorenz I want to do another PR to remove the extra text for the stdOut option, so that you can pipe it places. Any chance we can merge this first?

@thlorenz
Copy link
Owner

@RichardLitt since this is all related to the output, we could probably combine the two.
So you could just add those commits on top of this one and add to the title/description.

I assume you're gonna log on stderr to not clutter stdout, so stdout would only contain the output markdown right?
That's how I should've done it in the first place probably.

@RichardLitt
Copy link
Contributor Author

@thlorenz How about that?

Not sure about stderr -- where should that be applied, best?

Thanks.

@RichardLitt RichardLitt changed the title Updated help output Updated help output and removed extra logs from stdOut option Jun 24, 2016
@thlorenz
Copy link
Owner

Basically any console.log would become console.error except when you write markdown to the terminal.

@thlorenz
Copy link
Owner

So to be clear, removing logs when using stdout option isn't a good idea, but to make things pipeable you just log them on stderr so they print when you do doctoc --stdout > file.md but all markdown goes into file.txt since stdout is piped by default.

@RichardLitt
Copy link
Contributor Author

Might need some help with this one. Tried to get it working in this change. What do you think?

thlorenz
thlorenz reviewed Jun 25, 2016
View reviewed changes
doctoc.js Outdated
@@ -17,12 +17,12 @@ function cleanPath(path) {
}

function transformAndSave(files, mode, maxHeaderLevel, title, notitle, entryPrefix, stdOut) {
console.log('\n==================\n');
(stdOut) ? console.error('\n==================\n') : console.log('\n==================\n')
Copy link
Owner

Choose a reason for hiding this comment

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

Almost, but ALWAYS log on console.error not only if stdout is used.
It's the correct thing to do either way when logging (mistake of my past self for not doing so in the first place ;) )

@thlorenz
Copy link
Owner

@RichardLitt commented in one place on the main idea. This applies for all the other places as well.

Ideally you'd just take the original code and do a search replace %s/console.log/console.error/g and then make sure you use console.log ONLY for the markdown output.

@RichardLitt
Copy link
Contributor Author

RichardLitt commented Jun 26, 2016 •
edited
Loading

@thlorenz Ahh, got it. Pushed. Should be good?

@RichardLitt
Copy link
Contributor Author

@thlorenz Ping!

@RichardLitt
Copy link
Contributor Author

@thlorenz Any news on this? I still use this daily, and would love this change to be merged. :)

@thlorenz
Copy link
Owner

It LGTM in general, sorry for the delay.
Could you add some test where you launch doctoc as a child process and check what got printed to stdout and what to stderr? Also please add a section to the readme explaining the --stdout option.

@RichardLitt
Copy link
Contributor Author

I was pretty sure I had already done this; didn't #111 cover this? Why isn't this in the current package?

@RichardLitt
Copy link
Contributor Author

Update: It is in the current package, not sure why I hadn't seen it before. Works with a fresh install. So, this is just an update of that.

@thlorenz Do you still need the extra test? I don't see how that is different from the test I provided in #111 in transform-stdout.js. Happy to look again, though.

There already is a section on -s and --stdout in the README; do you want me to add more to it?

@RichardLitt
Copy link
Contributor Author

@thlorenz hey! Just checking old PRs and saw this. What do you think?

@RichardLitt
Copy link
Contributor Author

@thlorenz I'm curious. What have you moved onto? Do you still have a need to use this code, and are you still maintaining it?

@thlorenz
Copy link
Owner

thlorenz commented Aug 7, 2017

haven't moved on, just busy :)
However I still would like a test ...

Also any clarification as to how things work should be noted in the readme (if there is a section already add to/modify it please)

Thanks ..

Also would like to have other collabs give their OK before we merge this (hopefully they see this).

@RichardLitt
Copy link
Contributor Author

Updated! Let me know if this looks alright.

@thlorenz
Copy link
Owner

Thanks, on first glance looks great.
Will look this over next week in detail ...

1 similar comment
@kingbangla191
Copy link

Thanks, on first glance looks great.
Will look this over next week in detail ...

@PeterDaveHello PeterDaveHello requested review from AndrewSouthpaw and Copilot and removed request for AndrewSouthpaw November 20, 2024 17:00
Copilot
Copilot AI reviewed Nov 20, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • lib/transform.js: Evaluated as low risk
Comments skipped due to low confidence (3)

doctoc.js:25

  • The parameter name 'stdOut' is inconsistent with the typical naming convention. It should be renamed to 'stdout'.
result = transform(content, mode, maxHeaderLevel, title, notitle, entryPrefix, stdOut);

test/transform-stdout.js:17

  • The error message 'spits out extra lines as errors' is unclear. It should be updated to 'stderr should contain extra lines as errors'.
t.deepEqual(stderr, fs.readFileSync(__dirname + '/fixtures/stderr.md', 'utf8'), 'spits out extra lines as errors')

test/transform-stdout.js:26

  • The error message 'logs the correct table of contents' is unclear. It should be updated to 'stdout should contain the correct table of contents'.
t.deepEqual([ '- [Installation](#installation)', '- [API](#api)', '- [License](#license)', '' ].join('\n'), fs.readFileSync(__dirname + '/fixtures/stdout.md', 'utf8'), 'logs the correct table of contents')
@AndrewSouthpaw
Copy link
Collaborator

@PeterDaveHello if you would like this PR to proceed, feel free to contribute, the merge conflicts will need to be resolved first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@AndrewSouthpaw AndrewSouthpaw Awaiting requested review from AndrewSouthpaw

@thlorenz thlorenz Awaiting requested review from thlorenz

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

Successfully merging this pull request may close these issues.
4 participants
@RichardLitt @thlorenz @kingbangla191 @AndrewSouthpaw