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

feat(cordyceps): added new push_back and pop_front methods to list #198

Merged
merged 5 commits into from
Jun 5, 2022

Conversation

Tyrannican
Copy link
Contributor

This PR adds some nice-to-have methods as detailed in this issue.

Changes:

  • New push_back method
  • New pop_front method
  • Tests for each method

Small change but hope it's good :D

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

thanks for the PR! i have a couple of very minor suggestions:

  • the RustDoc for the List type references using the list as a FIFO using push_front and pop_back, it would be nice to add something saying that it can also be used as a stack or double-ended queue using these methods
  • thanks for adding tests! it could be good to add these methods to the proptest fuzz_linked_list test as well, but if that's too much work, it would be fine to do that in a follow-up branch.

thanks again for the PR!

cordyceps/src/list.rs Outdated Show resolved Hide resolved
hawkw and others added 2 commits June 5, 2022 11:14
…test. updated documentation to clarify the List can now be used as a stack or Doubly-Linked List
@Tyrannican
Copy link
Contributor Author

Tyrannican commented Jun 5, 2022

  • Added the tests to the proptest fuzz_linked_list
  • Added documentation to detail the List data structure can be used as a stack or Doubly Linked List

Edit: Committed these changes on my other account but it's still me!

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks great, i'm going to merge it once CI has run! thanks again!

@hawkw hawkw enabled auto-merge (squash) June 5, 2022 21:19
@hawkw hawkw disabled auto-merge June 5, 2022 21:19
@hawkw hawkw enabled auto-merge (squash) June 5, 2022 21:20
Copy link
Owner

@hawkw hawkw left a 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, i'm going to merge as soon as CI is done. thanks again!

@hawkw
Copy link
Owner

hawkw commented Jun 5, 2022

oh, looks like rustfmt also needs to be run: https://github.com/hawkw/mycelium/runs/6747421224?check_suite_focus=true

mind fixing that? thanks!

auto-merge was automatically disabled June 5, 2022 22:21

Head branch was pushed to by a user without write access

@Tyrannican
Copy link
Contributor Author

Done!

@hawkw hawkw enabled auto-merge (squash) June 5, 2022 22:27
@hawkw hawkw disabled auto-merge June 5, 2022 22:27
@hawkw hawkw enabled auto-merge (squash) June 5, 2022 22:28
@hawkw hawkw merged commit c555772 into hawkw:main Jun 5, 2022
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