Skip to content

Auto-format our XAML files and enforce in CI#9589

Merged
DHowett merged 32 commits intomainfrom
dev/migrie/xaml-formatting-2.0
Mar 29, 2021
Merged

Auto-format our XAML files and enforce in CI#9589
DHowett merged 32 commits intomainfrom
dev/migrie/xaml-formatting-2.0

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 23, 2021

This adds XamlStyler.Console to our solution, and calls it when we
format the code, to also format
our .xaml files.

  • XamlStyler.Console is a dotnet tool so it needs to be restored with
    dotnet tool restore
  • I've added a set of rules to approximately follow @cmaneu's XAML guidelines.
    Those guidelines also recommend things based on the code-behind, which
    this tool can't figure out, but also don't matter that much.
  • There's an extra step to strip BOMs from the output, since Xaml Styler
    adds a BOM by default. Some had them before and others didn't. BOMs
    have been nothing but trouble though.

@github-actions

This comment has been minimized.

  This should _not_ work guys
@github-actions

This comment has been minimized.

@zadjii-msft
Copy link
Member Author

HOOPLAH, it works in CI. I've got a bit more polish to do, so I'll tag folks when this is ready

@zadjii-msft zadjii-msft marked this pull request as draft March 23, 2021 15:43
@zadjii-msft zadjii-msft changed the title Just a check to see if this will work in CI Auto-format our XAML files and enforce in CI Mar 23, 2021
@zadjii-msft zadjii-msft marked this pull request as ready for review March 23, 2021 16:46
@zadjii-msft zadjii-msft added Area-Build Issues pertaining to the build system, CI, infrastructure, meta Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Mar 23, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

THANK YOU!

Does VS/VSCode automatically adhere to this styling, or do we need to do anything for that?

@carlos-zamora carlos-zamora removed their assignment Mar 23, 2021
@zadjii-msft
Copy link
Member Author

zadjii-msft commented Mar 23, 2021

Does VS/VSCode automatically adhere to this styling, or do we need to do anything for that?

lol like VS adheres to any sort of consistent rules 😝

That I'm still working on. Probably not? Every halfway decent text editor has some sort of "on save" hook that you can run scripts in, so I'll need to find ways of attaching there. THIS IS APPARENTLY NOT AN ACCURATE STATEMENT.

@zadjii-msft
Copy link
Member Author

Does VS/VSCode automatically adhere to this styling, or do we need to do anything for that?

That I'm still working on.

I can get you a git hook. That'll update the file silently as you commit the file. Sound good?

@zadjii-msft
Copy link
Member Author

Stick the following in .git/pre-commit:

################################################################################
# XAML Styler - xstyler.exe pre-commit Git Hook
# Documentation: https://github.com/Xavalon/XamlStyler/wiki
# Originally from https://github.com/Xavalon/XamlStyler/wiki/Git-Hook

# Define path to xstyler.exe
XSTYLER_PATH="dotnet tool run xstyler --"

# Define path to XAML Styler configuration
XSTYLER_CONFIG=

# Define path to copy original XAML files as backup
# BACKUP_PATH=.XamlBackup

echo "Running XAML Styler on committed XAML files"
git diff --cached --name-only --diff-filter=ACM  | grep -e '\.xaml$' | \
# Wrap in brackets to preserve variable through loop
{
    # Setup XAML file backup
    # if [ -n "$BACKUP_PATH" ]; then
    #     echo "Backing up XAML files to: $BACKUP_PATH"
    #     BACKUP_PATH="$BACKUP_PATH/$(date +"%Y-%m-%d_%H-%M-%S")/"
    # fi

    files=""
    # Build list of files to pass to xstyler.exe
    while read FILE; do
        if [ "$files" == "" ]; then
            files="$FILE";
            # mkdir -p $BACKUP_PATH
        else
            files="$files,$FILE";
        fi

        # if [ -n "$BACKUP_PATH" ]; then
        #     cp -r --parents $FILE $BACKUP_PATH
        # fi
    done

    if [ "$files" != "" ]; then
        # Check if external configuration is specified
        [ -z "$XSTYLER_CONFIG" ] && configParam="" || configParam="-c $XSTYLER_CONFIG"

        # Format XAML files
        $XSTYLER_PATH -f "$files" $configParam
        git add -u
    else
        echo "No XAML files detected in commit"
    fi

    exit 0
}
################################################################################

I commented out the backup functionality. You can keep that if you'd like.

@github-actions

This comment has been minimized.

@zadjii-msft
Copy link
Member Author

@carlos-zamora wait that git hook won't work right - it'll add all the BOMs back :(

  Presumably, git will fix the line endings in the repo and check out the right ones? lawd everything is awful
@zadjii-msft
Copy link
Member Author

Alright @DHowett, I figured all the bugs out. This is actually ready now.

@github-actions

This comment has been minimized.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 29, 2021
@ghost ghost requested review from DHowett, PankajBhojwani, leonMSFT and miniksa March 29, 2021 11:13
# format the files - it'll only ensure that they're formatted correctly.
function Invoke-VerifyXamlFormat() {
$root = Find-OpenConsoleRoot
& dotnet tool restore --add-source https://api.nuget.org/v3/index.json
Copy link
Member

Choose a reason for hiding this comment

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

Presumes a lot about the user's environment... hmm.

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 29, 2021
…tting-2.0

# Conflicts:
#	src/cascadia/TerminalApp/CommandPalette.xaml
#	src/cascadia/TerminalSettingsEditor/Launch.xaml
@zadjii-msft zadjii-msft requested a review from DHowett March 29, 2021 22:00
@DHowett
Copy link
Member

DHowett commented Mar 29, 2021

However, the xaml formatter hates itself. Ah, Me asking you to rename the thing made us break it. I'm sorry.

@DHowett
Copy link
Member

DHowett commented Mar 29, 2021

It needs to be exported from the thing

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

Labels

Area-Build Issues pertaining to the build system, CI, infrastructure, meta Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants