Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added Create TOC without backup option --ins-nb #70

Merged
merged 2 commits into from
Apr 28, 2020

Conversation

jordantrizz
Copy link
Contributor

First time doing this, hopefully, you consider it for merging.

I tried to do a sed append, however, OSX sed requires a new line. It got really tricky and I abandoned it in favor of just generating the TOC files and deleting them. Dirty but works and keeps your repository clean of backup files.

@Potherca
Copy link

It looks like you inadvertantly broke some of the test... You might want to look into that.

@jordantrizz
Copy link
Contributor Author

So it looks like this wasn't my mistake? But rather the Docker section wasn't added properly. I've addressed this as a new commit on my forked branch. Don't know how long it takes to show up here.

@jordantrizz jordantrizz changed the title Added --nobackup-insert option Added no backup grenated option --ins-nb Nov 25, 2019
@jordantrizz jordantrizz changed the title Added no backup grenated option --ins-nb Added Create TOC without backup option --ins-nb Nov 25, 2019
@jordantrizz
Copy link
Contributor Author

Pushed new code that passes tests.

@jordantrizz jordantrizz reopened this Nov 25, 2019
@jordantrizz
Copy link
Contributor Author

Running the test locally doesn't fail, I don't know why this is failing on travis? @Potherca

@Potherca
Copy link

Looks like a glitch. I've got the test passing locally too. I've triggered a separate Travis build on my own fork for the changes in this PR and that passes: https://travis-ci.org/potherca-contrib/github-markdown-toc/builds/617162196

image

I don't have access rights but maybe @ekalinin can manually trigger the build again?

@jordantrizz
Copy link
Contributor Author

Looks like a glitch. I've got the test passing locally too. I've triggered a separate Travis build on my own fork for the changes in this PR and that passes: https://travis-ci.org/potherca-contrib/github-markdown-toc/builds/617162196

I don't have access rights but maybe @ekalinin can manually trigger the build again?

Thanks!

@ekalinin
Copy link
Owner

Thanks for your contribution!

@jordantrizz jordantrizz force-pushed the nobackup branch 5 times, most recently from c653c35 to e203ebc Compare January 10, 2020 23:23
@jordantrizz
Copy link
Contributor Author

I've made the changes you requested, hopefully, they're correct. I'm sorry the last request was so sloppy, I apologize!

I wasn't certain how you wanted the usage to be printed out, right now it's kinda ugly due to --no-backup being longer than --insert.

Also, I wasn't certain if you wanted --no-backup to require --insert as an argument as well. Which I'm thinking you did. Let me know and I'll fix it so that --insert is required when using --no-backup.

@jonahx
Copy link

jonahx commented Apr 23, 2020

Are you still planning to merge this?

@jordantrizz
Copy link
Contributor Author

@jonahx it's possible @ekalinin just forgot about this one. I don't know how to poke him.

@ekalinin
Copy link
Owner

Sorry for delay. Please, check my last comment.
Let's just fix the help (or option name) and I will merge this PR.

@jordantrizz jordantrizz force-pushed the nobackup branch 2 times, most recently from 4985dea to 38e062a Compare April 23, 2020 16:16
@jordantrizz
Copy link
Contributor Author

This was my fault, I missed some things. I think I've gotten everything this time @ekalinin

@jordantrizz
Copy link
Contributor Author

Missed some the tests! Changed them as well.

@jonahx
Copy link

jonahx commented Apr 23, 2020

Thank you both!

@jordantrizz jordantrizz force-pushed the nobackup branch 2 times, most recently from 18850a0 to c9960fe Compare April 24, 2020 19:52
@jordantrizz
Copy link
Contributor Author

Alright, little learning lesson here on how to amend pull requests. Sorry about that @ekalinin I think I've made the changes you wanted, can you confirm?

@ekalinin
Copy link
Owner

@jordantrizz we're almost done :)

@jordantrizz
Copy link
Contributor Author

Removed the debug line!

@ekalinin ekalinin merged commit 76c9a6c into ekalinin:master Apr 28, 2020
@ekalinin
Copy link
Owner

Cool! Thanks!

@jonahx
Copy link

jonahx commented Apr 28, 2020

Thanks to you both!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants