Skip to content

Removed tabs and replaced with spaces, fixed error with $no_backup wa…#85

Closed
jordantrizz wants to merge 1 commit into
ekalinin:masterfrom
jordantrizz:default-file
Closed

Removed tabs and replaced with spaces, fixed error with $no_backup wa…#85
jordantrizz wants to merge 1 commit into
ekalinin:masterfrom
jordantrizz:default-file

Conversation

@jordantrizz
Copy link
Copy Markdown
Contributor

There were some leftover tabs that I turned into spaces, some were from my previous comment and some weren't.

An error was generated when using --insert due to $no_backup not being defined. Fixed this by adding $no_backup=no when using --insert

This revealed another issue with checking if $no_backup is null, changed to if = no.

When no "src" was defined on the command line, the script would simply print "Created by gh-md-toc" which isn't helpful. Instead put in the following check, if $src is null utilize README.md as the default file to use. If it doesn't exist it will fail with file not found.

…sn't defined, added in default file README.md when no $src is defined
Comment thread gh-md-toc

if [ "$1" = '--insert' ]; then
need_replace="yes"
no_backup="yes"
Copy link
Copy Markdown

@axeljonson axeljonson Jul 20, 2020

Choose a reason for hiding this comment

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

Hi @jordantrizz I don't think this going to get you what you are expecting. Placing no_backup="yes" on line 272 will cause the "--insert" and "--no_backup" CLI options to be dependent and that may not be what folks want or expect. Just because I want to insert the TOC does not necessarily mean I don't want a backup. To keep them independent, you may want to remove this line and add the following:
line 233: local no_backup="no"
And then change line 182 as follows:
from:
if [ -z $no_backup ]; then
to:
if [ $no_backup = "no" ], then

I think this will keep the two CLI options independent and the code will provide the expected results.

Copy link
Copy Markdown
Contributor Author

@jordantrizz jordantrizz Nov 27, 2020

Choose a reason for hiding this comment

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

Totally, wish I had time to commit to this but I don't :(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This has been fixed differently in #108

@ekalinin
Copy link
Copy Markdown
Owner

Please, fix the conflict.

@jordantrizz
Copy link
Copy Markdown
Contributor Author

Please, fix the conflict.

If I get some spare cycles I will be able to address the issue. Sorry!

@ekalinin ekalinin closed this Feb 2, 2021
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.

4 participants