ethz-asl / data-driven-dynamics

Data Driven Dynamics Modeling for Aerial Vehicles
Other
94 stars 13 forks source link

style: add python formatter #222

Closed sjschlapbach closed 1 year ago

sjschlapbach commented 1 year ago

Problem Description: Until now, different contributors used different Python formatters, what can lead to significant git diffs without having changes most sections of the code.

Proposed Solution: This PR includes the settings for the VS-Code setup to format all files automatically with Black (as discussed in issue #221) and applies this new formatter rule to all existing files.

sjschlapbach commented 1 year ago

LGTM! Thanks!

Is there an easy way to lint the code locally? This would be helpful to commit formatted code

@Jaeyoung-Lim If you use VS Code, the current settings configuration should ensure that formatting is automatically performed on save when the black formatter plugin is installed. Otherwise, files can be easily formatted using black from the command line (https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html). To only lint the code, you can run black --check from the command line :)

manumerous commented 1 year ago

Thanks for the great effort @sjschlapbach ! Much appreciated!

A detail regarding the PR. I would actually prefer a solution that does not not rely on vscode to perform the formatting (Maybe somebody is using pycharm, VIM, sublime ect.). Also within vscode it would be nice to allow the users to have different configurations as long as they conform to the style before they commit.

Therefore I would propose to add the black style checks to fix_code_style.sh similarly to how it is done in the visual dataframe selector. This way the user has a single script to format all the code in the repository while having the flexibility of choosing the text editor or IDE of their choice.

Do you agree @sjschlapbach? If yes would you maybe have the time to adapt this in a new PR? Else I can do it. Thanks for the great initiative anyways!

sjschlapbach commented 1 year ago

@manumerous Yes, I absolutely agree, having settings for other editors as well is definitely a good idea. Until now, I didn't implement it as this would probably require the user to install black via pip as well, which is not necessary when using VS Code (with the plugin).

However, if we want to extend the fix_code_style.sh file anyway, it might be an idea to automatically run this on every commit to enforce the correct formatting automatically? Otherwise, with the settings file removed again from the git tree and no pre-commit hooks, wrongly formatted code will only be detected in the CI actions after pushing. Was this the solution you already implemented in the visual dataframe selector? In this case, it would probably be faster for you to quickly fix this, instead of me looking into your solution and then building something similar?

Pushing VS Code setting-files to remote repos is always a big discussion - some like it, others hate it ^^ I'm open to any better solutions :)

manumerous commented 1 year ago

Ok I see. If you do not need to install black tgat is of course an advantage.

If we add it to the shell script I would just add black to the requirements.txt

I n just implemented the formatting tin the script but did not execute it automatically when comitting in the visual data frame selector. It would also be nice to exclude submodules from the formatting. We could for example add them to a libs folder a d then exclude that in the shell script. I will think of a solution sometimes next week. Let me know if you have any good ideas to share 😀

sjschlapbach commented 1 year ago

Sounds great 👍 , thanks! If you need any help with something specific / where more effort is needed, I can probably also pitch in some time