Skip to content

/aut

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

Rationalize computeHash and ComputeMD5 #333

Closed
jrwiebe opened this issue Jul 30, 2019 · 5 comments

Comments

@jrwiebe
Copy link
Contributor

commented Jul 30, 2019

I suggested on Slack that we rationalize the naming of the matchbox.package method computeHash and the ComputeMD5 object. Or eliminate one. (I vote to keep the ComputeMD5 object.) I recently pushed a change that makes the output of these identical.

@ruebot @lintool
@ruebot

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 30, 2019

Is it just this For string data, it is better to use StringUtils.computeHash().? One is for bytes and one is for strings?

Does it matter performance-wise just picking one?
@jrwiebe

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jul 30, 2019

computeHash just converts the string to a byte array with s.getBytes.
@ruebot

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 30, 2019

@lintool any objection to getting rid of computeHash and replacing it with ComputeMD5? I'm happy to do the work if you're good with it.

ruebot added a commit that referenced this issue Jul 30, 2019

@ruebot
Update tests for @jrwiebe recent commits; see also #333.
Verified
This commit was signed with a verified signature.
ruebot Nick Ruest
GPG key ID: 417FAF1A0E1080CD Learn about signing commits
09e98e2
@greebie

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jul 30, 2019

I think I added computeHash as a quick (and hacky) way to match ids in networks before a better method was used in #289 (preferring .zipWithIndex). I cannot recall if we are still using the old way in any remaining functions -- I think maybe we deprecated them? Either way, I think it is a good job for a code reaper.
@ianmilligan1

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 30, 2019

I think it is a good job for a code reaper.

Agreed – and thanks so much for the context on this, @greebie! This all rings a bell now and a search of the code bears this out.

ruebot added a commit that referenced this issue Aug 7, 2019

@ruebot
Replace computeHash with ComputeMD5; resolves #333.
Verified
This commit was signed with a verified signature.
ruebot Nick Ruest
GPG key ID: 417FAF1A0E1080CD Learn about signing commits
Loading status checks…
51ad2be

@ruebot ruebot referenced this issue Aug 7, 2019

Merged

Replace computeHash with ComputeMD5; resolves #333. #338

@jrwiebe jrwiebe closed this in 9623c7a Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jrwiebe @ruebot @greebie @ianmilligan1
You can’t perform that action at this time.