Skip to content

Conversation

@maruilian11
Copy link

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.

Copy link
Owner

@PeterDaveHello PeterDaveHello left a comment

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.

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
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
Owner

Ping @maruilian11

@maruilian11
Copy link
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
Owner

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

@maruilian11
Copy link
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
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
Author

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

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
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
Author

@PeterDaveHello Updated.

Copy link
Owner

@PeterDaveHello PeterDaveHello left a comment

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
Copy link
Author

maruilian11 commented Oct 15, 2017

@PeterDaveHello Just updated.

Copy link
Owner

@PeterDaveHello PeterDaveHello left a comment

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?

Copy link
Author

@maruilian11 maruilian11 left a comment

Choose a reason for hiding this comment

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

How about this?

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
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.

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
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.

Copy link
Author

@maruilian11 maruilian11 Nov 23, 2017

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.

Copy link
Author

@maruilian11 maruilian11 Nov 23, 2017

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.

Copy link
Owner

@PeterDaveHello PeterDaveHello left a comment

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?

Copy link
Owner

@PeterDaveHello PeterDaveHello left a comment

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