Skip to content

Clippy#39

Closed
lespea wants to merge 2 commits into
bootandy:masterfrom
lespea:clippy
Closed

Clippy#39
lespea wants to merge 2 commits into
bootandy:masterfrom
lespea:clippy

Conversation

@lespea

@lespea lespea commented Nov 21, 2019

Copy link
Copy Markdown
Contributor

Fixes a bunch of clippy warnings which includes:

  • Don't box the Nodes (not needed since it's inside a vec)
  • Reversing the node children no longer requires collecting everything into an intermediary vec

Let me know if some of these changes you don't want (specifically the collapsed if-then-else branches you may prefer the original... I just went for cleaning everything for a first pass.

@bootandy

Copy link
Copy Markdown
Owner

Hi, Firstly thanks for the pull request.

I like most of your changes but there are 2 things I'd like to change:

The collapsed if-then-else branches - I would like to leave these as they were as it is easier to understand them (we could put them in to two functions if you'd prefer).

Using the 'either' crate. To me it has hidden something in the way that 'get_children_from_node' works. I'd rather be more explicit even if I have to do a .map().collect() [If there are better ways to do this please feel free to suggest them].

But I like the other changes: removing box, changing iter to intoIter & getting rid of a few Somes.

So if you could undo the if/else squashing and the either crate I'd happily accept this merge.

thanks.

@lespea lespea closed this Dec 27, 2019
@lespea lespea deleted the clippy branch December 27, 2019 19:26
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