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

Nielsmeima/flatten chunk #56

Merged

Conversation

nielsmeima
Copy link
Contributor

@ncpenke I took a stab. Could you review this?

@ncpenke
Copy link
Collaborator

ncpenke commented Jul 1, 2022

Looks amazing, thanks also for adding such thorough unit tests! I think just a few things needed to fix up the checks:

  1. Run rust fmt rust fmt --all
  2. Run clippy cargo clippy --all --tests --fix
  3. Check that there aren't any compilation warnings (I think that's why the tests are failing).

Otherwise looks great!

@nielsmeima
Copy link
Contributor Author

Code has been run through fmt and clippy. Remaining warning seems to originate from an unused lifetime in deserialize.rs. Also, a test in tests/ui/struct_incorrect_type.rs seems to be failing. However, both are outside the scope of changes of this PR.

@ncpenke
Copy link
Collaborator

ncpenke commented Jul 2, 2022

Thanks for fixing up the formatting and clippy @nielsmeima. Taking a look at the warnings. main is successful so that's why thought the failures were related to these changes but could be something else.

@jorgecarleitao
Copy link
Collaborator

the new version of stable rust 1.62 landed yesterday which introduced new lints, so lint warnings are to be expected (almost everywhere in the ecosystem xD)

@ncpenke
Copy link
Collaborator

ncpenke commented Jul 2, 2022

Thanks, I don't know how long it would've taken me to figure that out! Thanks for the great work @nielsmeima, merging this!

@ncpenke ncpenke merged commit af9c3d0 into DataEngineeringLabs:main Jul 2, 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.

None yet

3 participants