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

parse_yaml.py: keep the trailing newline #159

Merged
merged 1 commit into from
Dec 21, 2023
Merged

parse_yaml.py: keep the trailing newline #159

merged 1 commit into from
Dec 21, 2023

Conversation

BrunoB81HK
Copy link
Contributor

Hi!

I have been using your library for a while and it's really great!

I have a very particular issue that is probably very niche, but I decided to try and fix it. As you can see in the pull request, the fix is very simple.

My use case is very particular, I use docker to prebuild my workspace and then I hop into the container to develop. When prebuilding with --symlink-install, this package create the module inside my src/ directory.

The problem arise when I mount my workspace inside the container. It essentially wipe the generated modules. The solution I use is to rebuild the packages inside my container and then commit the generated modules.

Which comes to the main point of this pull request, the trailing new line. Since most linter and formatter enforce a newline at the end of a python file by default, the fact that the generated modules don't have one is a little bit annoying.

tl;dr: For the (few) packages that somehow commit the generated modules, adding a trailing newline to the generated module could be a nice quality of life upgrade.

@pac48
Copy link
Collaborator

pac48 commented Dec 17, 2023

@BrunoB81HK Can you run pre-commit to format your change pre-commit run -a. Otherwise, it looks good.

@BrunoB81HK
Copy link
Contributor Author

BrunoB81HK commented Dec 18, 2023

@pac48 I'm so used to it being installed on my repo that I forgot to install the hook before committing. This is done!

@pac48
Copy link
Collaborator

pac48 commented Dec 19, 2023

@BrunoB81HK Looks good now. I will merge it if you can fix the merge conflict with main.

@BrunoB81HK
Copy link
Contributor Author

@pac48 I think its done

@tylerjw
Copy link
Contributor

tylerjw commented Dec 19, 2023

pre-commit is still failing with a formatting issue, do you mind running that and pushing the change?

@BrunoB81HK
Copy link
Contributor Author

@tylerjw I don't mind. I will do it tomorrow.

@BrunoB81HK
Copy link
Contributor Author

@pac48 @tylerjw Since the main branch was updated and it included changes to the .pre-commit-config.yaml file, the PR's commits were all over the place. I rebased the branch onto the latest commit on main, ran pre-commit and squashed all my commits into one. It should now be resolved.

@tylerjw
Copy link
Contributor

tylerjw commented Dec 20, 2023

I'm sorry for the difficulty getting this merged. I just kicked off ci again and will merge when it passes.

@tylerjw tylerjw merged commit 2734b70 into PickNikRobotics:main Dec 21, 2023
7 checks passed
@BrunoB81HK
Copy link
Contributor Author

Great! Thanks a lot guys for your time!

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