Skip to content
This repository has been archived by the owner on Feb 8, 2022. It is now read-only.

Port update cells command #891

Merged
merged 4 commits into from
Jan 30, 2022
Merged

Conversation

rusty-jules
Copy link
Contributor

Description

Hello! I have done a port of update cells, even though I did not sign up for it. This was really just a learning experience for me to dip my toe into contributing to nushell (I love this tool!!), but only after writing this did I notice that this port had been assigned to @kubouch (I'm assuming) in nushell/nushell#4356, although it had not been assigned in #242 which is what I had been looking at. Did not mean to not follow protocol so I don't mind closing if this is unhelpful.

Tests

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --all --all-features -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style <-- this failed here
  • cargo build; cargo test --all --all-features to check that all the tests pass

Notes

I really went into this blind and probably did some things in a non-idiomatic way, but the tests from the original are passing. There are currently two FIXMEs [one, two] where I had questions.

Clean up, nicer match statements in UpdateCellsIterator

Return columns flag into HashSet errors

Add FIXME: for update cell behavior on nested lists
@kubouch
Copy link
Contributor

kubouch commented Jan 30, 2022

Haha, that's fine, I haven't started yet! Thanks for the effort!

@rusty-jules
Copy link
Contributor Author

rusty-jules commented Jan 30, 2022

Whoops, a last minute attempt to make the code prettier broke it. I've reverted it back to a less-pretty if statement. I promise it passes this time! So much for a good first impression 😅

@sophiajt
Copy link
Contributor

@rusty-jules - heh, no worries. For that clippy error, you'll need to update your code following clippy's suggestions. Instead of unwrapping, we have a standard error we use everywhere: ShellError. You can check for the possible error and return ShellError, or, if the error is impossible, use .expect. The other clippy warning I think you can turn your .into_iter() to just .iter()

@kubouch
Copy link
Contributor

kubouch commented Jan 30, 2022

@rusty-jules I left a comment. Otherwise looks good to me.

@kubouch
Copy link
Contributor

kubouch commented Jan 30, 2022

Looks good thanks a lot!

@kubouch kubouch merged commit 67cb720 into nushell:main Jan 30, 2022
@rusty-jules rusty-jules deleted the port-update-cells branch January 30, 2022 21:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants