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

remove dead code #584

Merged
merged 2 commits into from
Nov 1, 2023
Merged

remove dead code #584

merged 2 commits into from
Nov 1, 2023

Conversation

kickster97
Copy link
Member

@kickster97 kickster97 commented Oct 26, 2023

WHAT is this pull request doing?

Removes unreachable and dead code found with crystal tool unreachable src/lavinmq.cr

Disclaimer: I have done this mainly guided by specs and the crystal tooling. I have also run snowstorm for image: cloudamqp/lavinmq:pr-584
Might be good to have someone on Linux and Windows to try out locally

HOW can this pull request be tested?

Run crystal spec
Run snowstorm with image: cloudamqp/lavinmq:pr-584

@kickster97 kickster97 marked this pull request as ready for review October 26, 2023 15:39
src/lavinmq/mfile.cr Outdated Show resolved Hide resolved
src/lavinmq/mfile.cr Outdated Show resolved Hide resolved
src/lavinmq/mfile.cr Outdated Show resolved Hide resolved
@@ -40,12 +40,6 @@ module LavinMQ
end
end

def each(&)
Copy link
Member

Choose a reason for hiding this comment

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

Because forward_missing_to further up? Maybe remove that instead

Copy link
Member Author

Choose a reason for hiding this comment

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

we currently use each_value for shovels, instead of its own each
why remove forward_missing_to and exchange each_value for each instead of just removing each?

else raise UnknownHashAlgoritm.new
end
end

class UnknownHashAlgoritm < Exception; end
Copy link
Member

Choose a reason for hiding this comment

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

Then drop this too i guess

Copy link
Member Author

Choose a reason for hiding this comment

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

Still used higher up in file

src/lavinmq/mfile.cr Outdated Show resolved Hide resolved
src/stdlib/slice.cr Outdated Show resolved Hide resolved
@kickster97 kickster97 merged commit 4c7bf8a into main Nov 1, 2023
29 of 42 checks passed
@kickster97 kickster97 deleted the remove_unreachables branch November 1, 2023 07:44
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.

3 participants