Skip to content

Add echo about why files don't need to be minified - #15

Open
maruilian11 wants to merge 1 commit into
PeterDaveHello:masterfrom
maruilian11:patch-1
Open

Add echo about why files don't need to be minified#15
maruilian11 wants to merge 1 commit into
PeterDaveHello:masterfrom
maruilian11:patch-1

Conversation

@maruilian11

Copy link
Copy Markdown

Sometimes the file won't get minified file after running the script, I think it's better to tell people the reason.
I use yellow echo to explain it.

@PeterDaveHello PeterDaveHello left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This prompt is not good enough as it didn't tell the precise reason, there are two conditions.

Comment thread minify.sh Outdated
elif [ $(wc -l "$filename.$filetype" | awk '{print $1}') -ge 15 ] || [ $(grep -E $'^(\t|\ )' "$filename.$filetype" | wc -l) -ge 5 ]; then
do_min=1
else
echo.Yellow "$filename.$filetype is small enough, it don't need to be minified."

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it don't -> it doesn't

@PeterDaveHello

Copy link
Copy Markdown
Owner

Ping @maruilian11

@maruilian11

Copy link
Copy Markdown
Author

How about

echo.Yellow "$filename.$filetype doesn't have more than 15 lines. \nOr it doesn't have more than 5 lines with tab or blank in the head of sentence. \nIt is small enough, it doesn't need to be minified."

@PeterDaveHello

Copy link
Copy Markdown
Owner

There is no need to combine multi-line output in a single command here.

@maruilian11

Copy link
Copy Markdown
Author

Do you mean this?

echo.Yellow "$filename.$filetype doesn't have more than 15 lines."
echo.Yellow "Or it doesn't have more than 5 lines with tab or blank in the head of sentence."
echo.Yellow "It is small enough, it doesn't need to be minified."

@PeterDaveHello

Copy link
Copy Markdown
Owner

Yes.

@maruilian11
maruilian11 force-pushed the patch-1 branch 4 times, most recently from 386357a to 49decd8 Compare October 7, 2017 17:59
@maruilian11

Copy link
Copy Markdown
Author

@PeterDaveHello I updated the commit. Please take a look. Thanks.

Comment thread minify.sh Outdated
else
echo.Red "$filename.$filetype is small enough, not be minified:"
echo.Yellow "It doesn't have more than 15 lines."
echo.Yellow "Or it doesn't have more than 5 lines with tab(s) or blank(s) in the head of sentences."

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Or it doesn't have more than 5 lines with tab(s) or blank(s) ?
The condition here should be and not or ...

@maruilian11
maruilian11 force-pushed the patch-1 branch 2 times, most recently from cd946ca to 74def9d Compare October 13, 2017 10:05
@maruilian11

Copy link
Copy Markdown
Author

@PeterDaveHello Updated.

@PeterDaveHello PeterDaveHello left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The wording/grammar looks very strange now, and what's the reason why to change its color? Shouldn't it be just yellow warning?

@maruilian11

maruilian11 commented Oct 15, 2017

Copy link
Copy Markdown
Author

@PeterDaveHello Just updated.

@PeterDaveHello PeterDaveHello left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Any possibility to improve the wording in warning to be more smooth?

@maruilian11 maruilian11 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

How about this?

Comment thread minify.sh
do_min=1
else
echo.Yellow "$filename.$filetype is small enough, not be minified:"
echo.Yellow "It doesn't have more than 15 lines."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's not over 15 lines.

Comment thread minify.sh
else
echo.Yellow "$filename.$filetype is small enough, not be minified:"
echo.Yellow "It doesn't have more than 15 lines."
echo.Yellow "Or it doesn't have more than 5 lines with tab(s) and blank(s) in the head of sentences."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Or it's not over 5 lines with tab(s) and blank(s) in the head of sentences.

@maruilian11 maruilian11 Nov 23, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Could be the following reasons:
It doesn't have more than 15 lines.
It's not over 5 lines with tab(s) in the head of sentences.
It's not over 5 lines with blank(s) in the head of sentences.

@maruilian11 maruilian11 Nov 23, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I prefer this one

Could be the following reasons:
It doesn't have more than 15 lines.
It's not over 5 lines with tab/blank(s) in the head of sentences.

@PeterDaveHello PeterDaveHello left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should it be separated as multiple sentences?

@PeterDaveHello PeterDaveHello left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

So what's the final version of the full message block? Confused here.

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