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

make format-xml to reformat, make hook to set up pre-commit hook #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kba
Copy link
Member

@kba kba commented Jul 13, 2021

With make format-xml all the DITA files are reformatted using xmlstarlet fo, much like @bertsky did in #20.

make hook installs a pre-commit hook that:

  • runs make format-xml
  • checks whether any reformatted XML files need to be committed
  • aborts the commit if that is the case - just call git commit again and the changed files will be added.

I actually had to disable the hook for this PR ;-)

Comment on lines +74 to +75
find . -path ./$(notdir $(DITA_OT_DIR)) -prune -false -o -name '*.dita' -a -not \( -size 0 \) |while read i;do \
echo xmlstarlet fo $$i > $$i.formatted && mv $$i.formatted $$i; \
Copy link

Choose a reason for hiding this comment

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

Forgive my being inquisitive, but what's -false -o for? IIUC, -prune always evaluates to true, then -false always to false, then -o is a disjunction, then comes -name. So IINM that together means the same as -prune -name '*.dita'.

For \( -size 0 \) there's also -empty BTW.

The shell's while loop could be rewritten as | xargs 'xmlstarlet fo {} | sponge {}' or | parallel -j 'xmlstarlet fo {} | sponge {}' BTW.

Copy link
Member Author

Choose a reason for hiding this comment

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

Inquisitiveness is appreciated here.

-prune -false -o is one of those snippets I have in muscle memory. It doesn't do the right thing without -false -o. I agree it should but it doesn't. find(1) is weird sometimes.

\( -size 0 \) == -empty thanks, I'll change it.

As for sponge/parallel: Thanks for the tip, sponge solves the redirection issue and your approach is probably more efficient, but the while loop is easier to debug. And as for "debugging" I just noticed I committed the version with echo. My bad, will fix.

#!/bin/bash

make format-xml
git add en de
Copy link

Choose a reason for hiding this comment

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

Isn't that problematic though? IIUC this would level-out any differences incurred from the auto-formatter by updating the index already. So now I cannot discern my (presumably manual) changes from the hooked changes anymore.

If you would just omit the git add, then I could still distinguish the index from the new state of the working directory (git diff them).

In fact, that difference is exactly what the line below should check on – instead of distrusting any changes to the *.dita files (which would prevent one from ever changing them manually again, short of circumventing the hook with --no-verify).

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that problematic though?

I do not see this as problematic, not being able to differentiate the staged and pre-commit changes. The convenience of just doing "git commit" again outweighs the loss of exact control IMHO.

However you're right, that this would prevent comitting ANY dita files which is of course not what I intended :) I'll adapt to the git diff, don't stage anything approach.

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