Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YETUS-896 Add an emoji column to the github vote table #67

Closed
wants to merge 1 commit into from

Conversation

infraio
Copy link

@infraio infraio commented Jul 23, 2019

No description provided.

@apache-yetus
Copy link

(!) A patch to the testing environment has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/1/console in case of problems.

@apache-yetus
Copy link

👍 +1 overall

Emoji Vote Subsystem Runtime Comment
👍 0 reexec 1205 Docker mode activated.
_ Prechecks _
👍 +1 dupname 0 No case conflicting files found.
👍 +1 @author 0 The patch does not contain any @author tags.
_ master Compile Tests _
_ Patch Compile Tests _
👍 +1 shellcheck 0 There were no new shellcheck issues.
👍 +1 shelldocs 0 There were no new shelldocs issues.
👍 +1 whitespace 0 The patch has no whitespace issues.
_ Other Tests _
👍 +1 asflicense 10 The patch does not generate ASF License warnings.
👍 1231
Subsystem Report/Notes
Docker Client=19.03.0 Server=19.03.0 base: https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/1/artifact/out/Dockerfile
GITHUB PR #67
Optional Tests dupname asflicense shellcheck shelldocs
uname Linux 91aca168d630 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality precommit/src/main/shell/personality/yetus.sh
git revision master / 6b94d9d
Max. process+thread count 46 (vs. ulimit of 2000)
modules C: precommit U: precommit
Console output https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/1/console
versions git=2.7.4 maven=3.3.9 shellcheck=0.6.0
Powered by Apache Yetus 0.11.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@busbey
Copy link
Contributor

busbey commented Jul 23, 2019

The vote column seems redundant with the emoji maybe?

Is it worth a cli option to pick between emoji + text vote or just text vote or just emoji for votes?

Also should 0 votes have no emoji or perhaps a different one?

@infraio
Copy link
Author

infraio commented Jul 23, 2019

The vote column seems redundant with the emoji maybe?

Is it worth a cli option to pick between emoji + text vote or just text vote or just emoji for votes?

Also should 0 votes have no emoji or perhaps a different one?

I prefer to only use emoji. It is enough to show the vote result.

@busbey
Copy link
Contributor

busbey commented Jul 23, 2019

Okay so how about an option that determines if the vote column is emoji or text?

else
echo "${TP_VOTE_TABLE[${i}]}" >> "${commentfile}"
echo "| :+1: ${TP_VOTE_TABLE[${i}]}" >> "${commentfile}"
Copy link
Contributor

Choose a reason for hiding this comment

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

There are three other values that need handling here, +0, -0, and 0. Marking them with the same emoji as +1 is very misleading.

Copy link
Author

Choose a reason for hiding this comment

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

If we use different emoji for these vote. I thought we should use emoji + text vote both.

@infraio
Copy link
Author

infraio commented Jul 24, 2019

The complete list of github markdown emoji markup can be found in https://gist.github.com/rxaviers/7360908

@infraio
Copy link
Author

infraio commented Jul 24, 2019

Okay so how about an option that determines if the vote column is emoji or text?

Add a with-emoji option, if with-emoji, then use emoji and vote text both. If not, just use the vote text.

@apache-yetus
Copy link

(!) A patch to the testing environment has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/2/console in case of problems.

@apache-yetus
Copy link

👍 +1 overall

Vote Subsystem Runtime Comment
0 reexec 2016 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
_ master Compile Tests _
_ Patch Compile Tests _
+1 shellcheck 1 There were no new shellcheck issues.
+1 shelldocs 1 There were no new shelldocs issues.
+1 whitespace 0 The patch has no whitespace issues.
_ Other Tests _
+1 asflicense 10 The patch does not generate ASF License warnings.
2045
Subsystem Report/Notes
Docker Client=19.03.0 Server=19.03.0 base: https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/2/artifact/out/Dockerfile
GITHUB PR #67
Optional Tests dupname asflicense shellcheck shelldocs
uname Linux 3a3de9025db3 4.15.0-54-generic #58-Ubuntu SMP Mon Jun 24 10:55:24 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality precommit/src/main/shell/personality/yetus.sh
git revision master / 6b94d9d
Max. process+thread count 41 (vs. ulimit of 2000)
modules C: precommit U: precommit
Console output https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/2/console
versions git=2.7.4 maven=3.3.9 shellcheck=0.6.0
Powered by Apache Yetus 0.11.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@infraio
Copy link
Author

infraio commented Jul 25, 2019

@busbey @aw-was-here Any more concerns?

@apache-yetus
Copy link

(!) A patch to the testing environment has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/3/console in case of problems.

@apache-yetus
Copy link

👍 +1 overall

Vote Subsystem Runtime Comment
0 reexec 1870 Docker mode activated.
_ Prechecks _
+1 dupname 0 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
_ master Compile Tests _
_ Patch Compile Tests _
+1 shellcheck 0 There were no new shellcheck issues.
+1 shelldocs 1 There were no new shelldocs issues.
+1 whitespace 0 The patch has no whitespace issues.
_ Other Tests _
+1 asflicense 9 The patch does not generate ASF License warnings.
1897
Subsystem Report/Notes
Docker Client=19.03.0 Server=19.03.0 base: https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/3/artifact/out/Dockerfile
GITHUB PR #67
Optional Tests dupname asflicense shellcheck shelldocs
uname Linux 950bccfc6c13 4.15.0-52-generic #56-Ubuntu SMP Tue Jun 4 22:49:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality precommit/src/main/shell/personality/yetus.sh
git revision master / 6b94d9d
Max. process+thread count 46 (vs. ulimit of 2000)
modules C: precommit U: precommit
Console output https://builds.apache.org/job/yetus-github-multibranch/job/PR-67/3/console
versions git=2.7.4 maven=3.3.9 shellcheck=0.6.0
Powered by Apache Yetus 0.11.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@infraio
Copy link
Author

infraio commented Jul 30, 2019

Ping @busbey

@Apache9
Copy link
Contributor

Apache9 commented Aug 2, 2019

I agree that the emoji is duplicated with the vote. I suggested that, we can introduce an option called --github-use-emoji, then we will just use the emoji instead of the old +1, -1 0, -0 and so on.

This is my implelmentation.

apache/hbase@8e99b85

I tried to use it in hbase first but it does not work...

@busbey
Copy link
Contributor

busbey commented Aug 9, 2019

Closed in favor of #68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants