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

add new lines to the start and end of each structured text section #23

Merged
merged 6 commits into from
Apr 10, 2024

Conversation

aeu485
Copy link
Contributor

@aeu485 aeu485 commented Nov 7, 2023

Addresses #10. Works upon ST declaration and implementations, with the --lines argument for specifying additional \n.
Two modifications occur from the use ElementTree.Write:

  • xml version line changes from double to single quotes and encoding is capitalized
    <?xml version="1.0" encoding="utf-8"?>
    becomes
    <?xml version='1.0' encoding='UTF-8'?>
  • Folder tags Id attributes lose a space at their end
    <Folder Name="internal" Id="{ad4fec99-008a-4786-b366-d1f41be17bb4}" />
    becomes
    <Folder Name="internal" Id="{ad4fec99-008a-4786-b366-d1f41be17bb4}"/>

@ZLLentz
Copy link
Member

ZLLentz commented Mar 27, 2024

This is well implemented and a good addition, thank you!
I'm stalling out a bit on review due to some busy season/staffing shortages, it will take me a bit longer to verify the versioneer update (I know it's stock and standard, but it's also a big diff and I can't accept it without reviewing it thoroughly)

@aeu485
Copy link
Contributor Author

aeu485 commented Mar 28, 2024

No problem. I removed the versioneer updates as you didn't update in your most recent PR either.

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

Things I like:

  • Works as advertised! (if you modify slightly to make the hook entry and the python file names match as per my other comment)
  • The resulting files are still readable by TwinCAT and compile normally

Things I'm not crazy about (still going to merge regardless):

  • Since lxml modifies the xml header, this actually applies to every POU on every commit as it continually fights TwinCAT. This isn't a unique problem to this PR though, the xml formatter has the same thing. Maybe some day I'll come back through and figure out how to (if it is possible?) to make lxml behave.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be named twincat_st_newline.py for setup.py to automatically pick it up and provide it to pre-commit as a console script. (The entry names are generically mapped to module names).

@@ -9,12 +9,13 @@ repos:
files: \.(TcPOU|TcDUT|TcGVL)$

- repo: https://github.com/pcdshub/pre-commit-hooks.git
rev: v1.4.0
rev: v1.5.0
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for updating these too!

@ZLLentz
Copy link
Member

ZLLentz commented Apr 10, 2024

merging and then I'm going to make a mini follow-up PR to rename the file so it will run from the pre-commit binary

thank you for your contribution and your patience!

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