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

Added pre-commit configuration file #41

Merged
merged 4 commits into from
Sep 19, 2024
Merged

Conversation

ndelima-ekumen
Copy link
Collaborator

Added a basic configuration file for pre-commit, I wanted to check first what hooks are required. I'll add the actions in the next commit after the hooks are defined. Also, issue #35 expects the project to be linted, I'd like to leave that for the next PR, once this workflow is set up.

Copy link
Member

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine.
Let's add documentation to install and configure pre-commit please.

@ndelima-ekumen
Copy link
Collaborator Author

I ran the linter and made the necessary changes, also ran the dataset generation, training, training_test, and the simulated and real pipelines.
I had to move some words from the Nvidia license to fit the 80-character rule, but I'm not sure if that's ok @agalbachicar.
I also deactivated the line length rule in the isaac_ws config.yml as many lines were too long, but there was a consistent comment per line of configuration, is that ok?

Copy link
Member

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ndelima-ekumen ndelima-ekumen merged commit f617c15 into main Sep 19, 2024
1 check passed
@ndelima-ekumen ndelima-ekumen deleted the ndelima/linter_setup branch September 19, 2024 19:03
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