sunchao / parquet-rs

Apache Parquet implementation in Rust
Apache License 2.0
149 stars 20 forks source link

Format the source code #70

Closed sadikovi closed 6 years ago

sadikovi commented 6 years ago

This PR formats the source code based on rustfmt rules, files were inspected manually, however. Most of the formatting is preserved.

sadikovi commented 6 years ago

This relates to #44.

sadikovi commented 6 years ago

@sunchao Could you review this PR when you have time? I use cargo fmt -- --write-mode=diff now, instead of travis-cargo. Thanks!

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 92.854% when pulling 0837219c71a039000d06026f777e20a876802de3 on sadikovi:rust-format into 1f64efebfbec5e0f16a919046b7baaba051414c4 on sunchao:master.

sadikovi commented 6 years ago

@sunchao I agree with your comments - I actually had to correct the formatted code in a couple of places as well. Let me review the changes again and see if I can tweak the configuration file to make rust format make changes less aggressively.

sadikovi commented 6 years ago

@sunchao I updated the rustfmt.toml to address most of your comments. I also inspected changes manually, and they look okay for the most part (~ 96%).

Could you have a look again and advice on the next steps, e.g. whether or not we even need this? Thanks!

sunchao commented 6 years ago

Hi @sadikovi : sorry for the late reply! Is there a way to just enforce max line width + tab width (also maybe the reordering on imports)? my concern is that it currently enforces too many rules and limits flexibility (e.g., sometimes it may be more concise to put everything in a single line but this will break it up into multiple lines for all cases).

sadikovi commented 6 years ago

You are right 100%, I thought about something along those lines. I will try updating the config to check line width, indentation and maybe couple of other basic rules, and disable the rest.

Will update today, thanks.

sadikovi commented 6 years ago

@sunchao I could not find the option that would allow to apply only a subset of rules, without applying the rest. It looks like rustfmt always applies all rules, but checks for values in provided configuration file, if options do not exist in the file, rustfmt uses default values for those options.

Possible solutions:

  1. Keep the current rustfmt configuration and changes
  2. Close PR and do not apply rustfmt
  3. Create a PR with manual formatting based on rustfmt (only imports, indentations and couple of other items)

Let me know which way you would prefer to go with. I personally think that choosing option 3 might be a good way to start, or we could close it for now, because we have more important stuff to fix.

sunchao commented 6 years ago

@sadikovi : I'm also in favor of option 3) for now. We can keep the issue open and come back later if there's better solution. I'm also going to look into why rustfmt always apply all rules.

Thanks for working on this!

sadikovi commented 6 years ago

@sunchao could you review this PR again? Thanks! I updated the code manually without any rust format, and this does not include any changes to the build.

sadikovi commented 6 years ago

Thanks @sunchao! Would you mind merging this PR, if it is okay? If there is anything you want me to change, let me know - I will update the code. Thanks.

sunchao commented 6 years ago

Merged! Thanks!, and sorry for the delay @sadikovi .

sadikovi commented 6 years ago

Thank you very much @sunchao for merging!