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

Include new prettify #95

Closed
alazzaro opened this issue Nov 19, 2018 · 10 comments
Closed

Include new prettify #95

alazzaro opened this issue Nov 19, 2018 · 10 comments
Assignees
Milestone

Comments

@alazzaro
Copy link
Member

New prettify from https://github.com/pseewald/fprettify as submodule
Unfortunately, the new prettify "forces" make to always run the target since it always updates the file, even there were no changes!

@alazzaro alazzaro self-assigned this Nov 19, 2018
@alazzaro alazzaro added this to the v2.0 milestone Nov 19, 2018
@alazzaro
Copy link
Member Author

Close with 972f09e

@dev-zero
Copy link
Contributor

we could integrate the fprettify via pre-commit instead?

@alazzaro
Copy link
Member Author

Sure, of course. My first step was to include it...

@pseewald
Copy link
Contributor

I would suggest imposing a less aggressive version of fprettify that only fixes indentation but otherwise leaves whitespace formatting intact (fprettify --disable-whitespace). Same for CP2K, but due to legacy-prettify and legacy-doxify we are not ready yet for such a change... It's much easier to justify and explain to external contributors and reduces prettify noise in commit history.

If we include fprettify via git submodule, can we use latest release instead of master branch? Or do I need to create a branch with releases only?

@dev-zero
Copy link
Contributor

we can use whatever you want to be used ;-)

@alazzaro
Copy link
Member Author

@pseewald @dev-zero What's the status of this issue?
My trick was to do this in the Makefile:

# Pretty function, check if the file requires update
define pretty_func
        @mkdir -p $(PRETTYOBJDIR)
        @rm -f $2
        $(TOOLSRC)/fprettify/fprettify.py --disable-whitespace -s $1 > $2
        @cmp -s $1 $2; \
        RETVAL=$$?; \
        if [ $$RETVAL -ne 0 ]; then \
            cp $2 $1; \
        fi
endef

but I think we can now remove it with the new prettifier, right?

@pseewald
Copy link
Contributor

I still need to fix pseewald/fprettify#34

@alazzaro
Copy link
Member Author

OK, then I will move this issue to v3. No rush.

@alazzaro alazzaro modified the milestones: v2.0, v3.0 Apr 17, 2019
@pseewald
Copy link
Contributor

I still need to fix pseewald/fprettify#34

This is fixed

@alazzaro
Copy link
Member Author

closed via #270

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

No branches or pull requests

3 participants