-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Make [emoji] implementation deterministic #103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me! Awesome work! 🙇♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just caught something with the help of @sokra "over my shoulder".
Instead of the .9999 approach, it needs to be more like https://github.com/webpack/loader-utils/blob/master/lib/getHashDigest.js where emoji is another base in addition to existing ones there.
Let me know if that makes sense @kohlikohl
@TheLarkInn I think that makes sense. I'll take another stab at it. |
dcb56b2
to
43f0889
Compare
@TheLarkInn @sokra Pushed an update. Is this what you had in mind? |
7ee5269
to
88497a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Just a few little code style comments
lib/getHashDigest.js
Outdated
return emojis.splice(0, maxLength).join(""); | ||
} | ||
|
||
function sliceOutput(output, digestType, maxLength) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simpily this could you:
- make
encodeBufferToBase
return an array. - Use
Array.prototype.slice
instead ofString.prototype.substring
to ensuremaxLength
- Convert array to string with join after that.
- Remove sliceEmojis, sliceOutput, emoji-regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the emoji-regex
package. I'd encourage to keep it as it is better in detecting the different code points of emojis. Here is an example https://runkit.com/kohlikohl/emoji-regex.
The simple regex seems to only detect emojis that have two code points. Flags for example have four so it would miss identify and split them.
Maybe it is not relevant in this case, if so let me know and I'll make the final change.
lib/getHashDigest.js
Outdated
@@ -36,15 +43,35 @@ function encodeBufferToBase(buffer, base) { | |||
return output; | |||
} | |||
|
|||
function getBase(digestType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simpily this could you:
- Add a
encodeTable
argument toencodeBufferToBase
- Add a
encodeBufferToDigestType
which callsencodeBufferToBase
with the correct arguments according todigestType
. - Remove emojis from the
baseEncodeTables
(looks weird)
lib/getHashDigest.js
Outdated
digestType === "base49" || digestType === "base52" || digestType === "base58" || | ||
digestType === "base62" || digestType === "base64") { | ||
return encodeBufferToBase(hash.digest(), digestType.substr(4)).substr(0, maxLength); | ||
if(digestType && !!baseEncodeTables[getBase(digestType)]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a const array of valid digestType
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the idea with usingbaseEncodeTables
as a const to not have to repeat the digestTypes. Lets see 😄
lib/getHashDigest.js
Outdated
const emojisList = require("emojis-list"); | ||
const emojiRegex = require("emoji-regex")(); | ||
|
||
const BASE_EMOJI = "baseEmoji"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just call it emoji
Requested changes pending, otherwise this is looking great so far. Excellent work @kohlikohl |
88497a3
to
5be0725
Compare
…a deterministic way
5be0725
to
c60ec70
Compare
Pushed the changes based on the comments. Please have another look @TheLarkInn @sokra As mentioned in the response to the comments, I'd encourage to keep the |
When this PR will be merged? :) |
@TheLarkInn @sokra Are you guys happy with this PR? Is it ready to be merged in? |
Would love to see this merged. @kohlikohl can i suggest fixing the coverage regression, as maybe the lack of a ✅ on the PR checks is halting the approvals? |
/cc @kohlikohl sorry for big delay, can you rebase feature? |
No valid anymore |
PR for issue #102.
Make use of hashing function to choose an emoji instead of randomly selecting one.
Fun fact: Not that this would be needed much but it supports 1k+ emojis per hash. 🚀