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

Always use Unix-style line endings in generator output #132

Merged
merged 3 commits into from
Jun 8, 2024

Conversation

nbes4
Copy link
Contributor

@nbes4 nbes4 commented May 31, 2024

This PR adds:

  • A centralized serialize() implementation.
    I think this one made sense since it was the same for Device, Symbol, Package and Component. If these implementations differ in the future we could remove it again, but for now it prevents some code duplication. (I wasn't sure how to name it and where to put it, I opted for serialize_common and put it in the outer common.py file, suggestions welcome)
  • Always use Unix-style line endings (LF) when serializing entities (should fix development on Windows, see discussion here)
  • Tests

I tried the librepcb-cli open-library --all --strict --check --minify-step . command before the changes in this PR and it gave me the "Non-canonical file". After applying the changes in this PR it seemed to work just fine! :-)

Edit: I verified the Tests on my Windows machine. They failed before applying the changes, but passed after applying the changes from this PR.

@nbes4 nbes4 marked this pull request as ready for review May 31, 2024 21:19
Copy link
Member

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Looks good to me! Only small nits from my side.

Did you test whether the test actually fails when running on Windows without the fix?

common.py Outdated Show resolved Hide resolved
entities/component.py Outdated Show resolved Hide resolved
@nbes4
Copy link
Contributor Author

nbes4 commented Jun 5, 2024

@dbrgn Thanks for the review, I implemented your feedback!

Did you test whether the test actually fails when running on Windows without the fix?

Yes, I forgot to mention that but I did run the tests on my Windows machine before applying the changes in this PR and they failed, but passed once I applied the changes 👍 I updated the initial PR description to make it more clear upfront 😄

@rnestler rnestler requested a review from dbrgn June 7, 2024 16:27
Copy link
Member

@rnestler rnestler left a comment

Choose a reason for hiding this comment

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

Maybe we should even add Windows to the testing matrix

@dbrgn dbrgn merged commit aab872e into LibrePCB:master Jun 8, 2024
4 checks passed
@nbes4
Copy link
Contributor Author

nbes4 commented Jun 8, 2024

Thanks for merging 🎉

@rnestler more coverage never hurts IMO 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants