Skip to content

Conversation

@yiranzai
Copy link

@yiranzai yiranzai commented Apr 9, 2021

--all Find all Markdown files for non-hidden folders
--auto Ignore ts/te tags, Automatically at the end/head of the file
--head The TOC is generated in the header of the file, requires --auto

 --auto                Ignore ts/te tags, Automatically at the end/head of the file
 --head                The TOC is generated in the header of the file, requires --auto
Copy link
Owner

@ekalinin ekalinin left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

Could you, please, check my comments.
And one more thing: could you add some tests for a new functionality?

#

gh_toc_version="0.7.0"
gh_toc_version="0.8.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Let's leave current value for now

echo "yes";;
*)
echo "no";;
https* | http*)
Copy link
Owner

Choose a reason for hiding this comment

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

Could you, please, revert these changes?
Thus, total diff will be much smaller and easier to review.
Thanks.

exit 1
fi
local toc=`echo "$rawhtml" | gh_toc_grab "$gh_src_copy"`
local toc=$(echo "$rawhtml" | gh_toc_grab "$gh_src_copy")
Copy link
Owner

Choose a reason for hiding this comment

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

What's the point to change ` to $(?
Without this change total diff will be much simpler.

Copy link
Author

Choose a reason for hiding this comment

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

# Find all Markdown files for non-hidden folders
#
all_md_filename=""
gh_all_md_filename() {
Copy link
Owner

Choose a reason for hiding this comment

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

Using globals is a bad pattern.

Could you, please, improve gh_all_md_filename and just return the result from the function (without using global variable all_md_filename).

Copy link
Author

Choose a reason for hiding this comment

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

You are right

local ts="<\!--ts-->"
local te="<\!--te-->"
local dt=`date +'%F_%H%M%S'`
local dt=$(date +'%F_%H%M%S')
Copy link
Owner

Choose a reason for hiding this comment

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

And again :) what's the point to change ` to $(?

Copy link
Author

Choose a reason for hiding this comment

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

local ext=".orig.${dt}"
local toc_path="${gh_src}.toc.${dt}"
local toc_footer="<!-- Added by: `whoami`, at: `date` -->"
local toc_footer="<!-- Added by: $(whoami), at: $(date) -->"
Copy link
Owner

Choose a reason for hiding this comment

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

And again :) what's the point to change ` to $(?

Copy link
Author

Choose a reason for hiding this comment

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

# clear old TOC
sed -i${ext} "/${ts}/,/${te}/{//!d;}" "$gh_src"
# create toc file
echo "${toc}" > "${toc_path}"
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please, revert back the origin formatting?
Without this change total diff will be much simpler.

echo "os: `lsb_release -d | cut -f 2`"
echo "kernel: `cat /proc/version`"
echo "shell: `$SHELL --version`"
echo "os: $(lsb_release -d | cut -f 2)"
Copy link
Owner

Choose a reason for hiding this comment

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

And again :) what's the point to change ` to $(?

Copy link
Author

Choose a reason for hiding this comment

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

@yiranzai
Copy link
Author

I could revert to these changes, but this project seems to need a shell style checker

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