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

Performance optimizations and style #2

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Performance optimizations and style #2

merged 1 commit into from
Mar 6, 2018

Conversation

kpumuk
Copy link
Contributor

@kpumuk kpumuk commented Mar 6, 2018

This PR includes:

  • SHA1 is only calculated once
  • Frozen string literal to make sure Ruby does not copy strings over and over again
  • Removal of @match_data, since we only use it to retrieve the counter. Caching the counter instead (this way we do not re-apply regex over and over again if there is no match [should not happen, but still])
  • Unnecessary begin/end in get_hashes

@philnash
Copy link
Owner

philnash commented Mar 6, 2018

Thanks! There's some good stuff here.

Going to merge this, but I really prefer the begin/rescue/end way of error handling. It's mainly just the way it looks, I like the intention of saying "here is where I am going to rescue errors" rather than surprising the reader after a potential source of error. I'm going to keep that like it is. Otherwise you definitely caught a good bunch of issues here. Thanks!

@kpumuk
Copy link
Contributor Author

kpumuk commented Mar 6, 2018

Ok, let me fix that

@philnash
Copy link
Owner

philnash commented Mar 6, 2018

Well, I rebased and messed up this PR. Any ideas on how to fix it? I want to attribute the PR to you, not just close this. Perhaps you can pull the latest in master and force push to this branch, which should trigger GitHub to recognise the merge. (And I won't rebase again.)

@kpumuk
Copy link
Contributor Author

kpumuk commented Mar 6, 2018

Sure.

git reset --hard 062843efcb22863727074f9dbcb9cde60864009c
git push -f

This will essentially discard all the commits you have made after the initial "Adding a changelog" (which is rebase + changelog update, which I pulled into this PR just now)

@kpumuk
Copy link
Contributor Author

kpumuk commented Mar 6, 2018

Another option is to close this PR, and I will work to resolve the conflicts with my other branches (this PR is not the last one :-) ). Please let me know which way do you prefer.

@philnash philnash merged commit cd7dd5e into philnash:master Mar 6, 2018
@kpumuk kpumuk deleted the dmytro/performance branch March 7, 2018 07:21
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