Skip to content

Enhancing... Color echo and code cleanups#12

Merged
sbimochan merged 4 commits intomasterfrom
enhancement
Jan 13, 2019
Merged

Enhancing... Color echo and code cleanups#12
sbimochan merged 4 commits intomasterfrom
enhancement

Conversation

@sbimochan
Copy link
Copy Markdown
Owner

@sbimochan sbimochan commented Jan 11, 2019

  • error handling with set -euo pipefail
  • Removed CURRENT_BRANCH_CMD
  • Color added for warnings and success
  • Removed eval
  • Cleanups

@sbimochan sbimochan added help wanted Extra attention is needed good first issue Good for newcomers enhancement New feature or request and removed good first issue Good for newcomers help wanted Extra attention is needed labels Jan 12, 2019
Comment thread commit Outdated

COMMIT_WITH_BRANCH="git commit -m \"$CURRENT_BRANCH: $1\""
DEFAULT_COMMIT="git commit -m \"$1\""
colorEcho "YELLOW" "Excluding branches from .ignore file."
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.

Not sure if we need any of this information.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Since there’re confusing conditions like default ignoring branches and branches to ignore from .ignore file, I thought user may want to know what commit was really made.

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.

The use will know from the commit message in the end.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yes but making it fancy 🤣

Comment thread commit Outdated
Comment thread commit
@@ -1,26 +1,36 @@
#!/usr/bin/env bash
set -euo pipefail
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 is good.

Comment thread commit Outdated
else
echo "Commiting with default branch"
eval $DEFAULT_COMMIT
colorEcho "GREEN" "Commit translation: \"$1\""
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.

why not move this to a function as well if we're doing it for everything else?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I don’t think function is needed now. What do you think about colorEcho and function?

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.

Maybe you should add screenshots to the PR. How it looks now. It's a good idea to get visual feedback with colors on how the commit message has been written

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

screen shot 2019-01-13 at 4 13 34 pm

Copy link
Copy Markdown
Contributor

@mesaugat mesaugat Jan 13, 2019

Choose a reason for hiding this comment

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

On the third line of the output you already know if smart-commit was useful or not. Thus, any verbosity is not required. If it was for debugging or anything else, it would have made sense. Keep it simple, it should be fine.

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.

The coloring is not required in my opinion. What you can do is add an additional flag to show these messages. By default, it should show none.

@sbimochan
Copy link
Copy Markdown
Owner Author

When current branch is in exclude list:

screen shot 2019-01-13 at 4 14 40 pm

When current branch is not in exclude list:

screen shot 2019-01-13 at 4 13 34 pm

@aviskarkc10
Copy link
Copy Markdown
Contributor

Like @mesaugat said, we don't need the "Excluding branches..." text

@sbimochan
Copy link
Copy Markdown
Owner Author

@aviskarkc10 okay. sounds great. so only the green part? so that it could highlight what was the message

@aviskarkc10
Copy link
Copy Markdown
Contributor

I am with Saugat dai on the on color thing as well. but doesn't matter to me which way you go.

@sbimochan
Copy link
Copy Markdown
Owner Author

On a dilemma

@sbimochan sbimochan added the help wanted Extra attention is needed label Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants