redhat-developer / vscode-xml

Editing XML in Visual Studio Code made easy
Eclipse Public License 2.0
254 stars 79 forks source link

editor.formatOnSaveMode option is not respected #992

Open BerserkWolfS opened 4 months ago

BerserkWolfS commented 4 months ago

Hello,

firstly, thanks for your great extension and the efforts you put into it, I found the new formatter efficient and easy to use and configure. I have a request though regarding the option editor.formatOnSave. In my team, we configure it to modifications only to ease our code reviews, but the extensions seems to modify the whole file. Is it a side effect, have I missed a setting or is it planned for a future release ? This is my vscode configuration:

Version: 1.87.2 Commit: 863d2581ecda6849923a2118d93a088b0745d9d6 Date: 2024-03-08T15:20:17.278Z Electron: 27.3.2 ElectronBuildId: 26836302 Chromium: 118.0.5993.159 Node.js: 18.17.1 V8: 11.8.172.18-electron.0 OS: Windows_NT x64 10.0.22621

XML extension: 0.26.1

Thanks

angelozerr commented 4 months ago

We are glad that vscode xml please you. Thanks for your feedback!

To be honnest with you I have never used this format on save options.

I dont think we have implemented something for this option.

When you activate this options it should format the whole file, right?

Im sorry I dont understand your problem?

BerserkWolfS commented 4 months ago

Hello Angelo,

Thanks for your quick reply. In fact, the editor.formatOnSaveMode option (I corrected the name in the title since I forgot the "Mode" part) is an enum that you can set to modifications, which means only the lines you modified in a file will be formatted. I checked the tooltip, it says that is requires source control, so maybe your extension may have to use this information somehow ? Or maybe it is provided by vscode api, I don't know.

For instance, let's says I have the following content in a xml file:

<example>
  <par> 'Content' </par>
  <par> 'Other' </par>
</example>

Then I make a small modification to the line containing the word 'Content'. Imagine I turned on the enforce quote style option to preferred in the xml extension, and my preference is to use double quotes. Then, only the modified line should be formatted, like this:

<example>
  <par> "Content" modified </par>
  <par> 'Other' </par>
</example>

I know it is not ideal because it means that the file will end up containing mixed style. But it is the responsibility of our team to either keep it like this to ease the code review later and ensure that the rest of the file is kept intact or to fully format it using the format file command on purpose.

Do you think you can either add an option or take into account the built-in editor.formatOnSaveMode ?

fbricon commented 4 months ago

It works for me (enforce quote style refers to quotes in attributes)

https://github.com/redhat-developer/vscode-xml/assets/148698/7de22219-524b-4896-a962-78451cce789d

BerserkWolfS commented 4 months ago

ok, so I had to complexify my example a little to reproduce the issue ^^.

Here is the new sample:

<?xml version="1.0" ?>
<example>
    <par attr='myattr2'> Other </par>
    <par attr='myattr3'>
        <subpar attr='mysubattr1'/>
        <subpar attr='mysubattr2'/>
    </par>
    <par attr='myattr1'> Content </par>
</example>

https://github.com/redhat-developer/vscode-xml/assets/5680798/873e9e07-c2c0-4ded-9b01-2f13739fe2fa

As you can see, the subpar are also formatted. Maybe it is a designed choice ?

fbricon commented 4 months ago

No that looks like a bug

fbricon commented 4 months ago

@angelozerr I'm adding the lemminx trace for posterity:

[Trace - 4:32:24 PM] Sending request 'textDocument/rangeFormatting - (66)'.
Params: {
    "textDocument": {
        "uri": "file:///Users/fbricon/Dev/souk/spring-petclinic/example.xml"
    },
    "range": {
        "start": {
            "line": 7,
            "character": 0
        },
        "end": {
            "line": 7,
            "character": 47
        }
    },
    "options": {
        "tabSize": 4,
        "insertSpaces": true,
        "trimFinalNewlines": true
    }
}

[Trace - 4:32:24 PM] Received response 'textDocument/rangeFormatting - (66)' in 1ms.
Result: [
    {
        "range": {
            "start": {
                "line": 4,
                "character": 21
            },
            "end": {
                "line": 4,
                "character": 22
            }
        },
        "newText": "\""
    },
    {
        "range": {
            "start": {
                "line": 4,
                "character": 32
            },
            "end": {
                "line": 4,
                "character": 33
            }
        },
        "newText": "\""
    },
    {
        "range": {
            "start": {
                "line": 4,
                "character": 33
            },
            "end": {
                "line": 4,
                "character": 33
            }
        },
        "newText": " "
    },
    {
        "range": {
            "start": {
                "line": 5,
                "character": 21
            },
            "end": {
                "line": 5,
                "character": 22
            }
        },
        "newText": "\""
    },
    {
        "range": {
            "start": {
                "line": 5,
                "character": 32
            },
            "end": {
                "line": 5,
                "character": 33
            }
        },
        "newText": "\""
    },
    {
        "range": {
            "start": {
                "line": 5,
                "character": 33
            },
            "end": {
                "line": 5,
                "character": 33
            }
        },
        "newText": " "
    },
    {
        "range": {
            "start": {
                "line": 7,
                "character": 14
            },
            "end": {
                "line": 7,
                "character": 15
            }
        },
        "newText": "\""
    },
    {
        "range": {
            "start": {
                "line": 7,
                "character": 22
            },
            "end": {
                "line": 7,
                "character": 23
            }
        },
        "newText": "\""
    }
]

[Trace - 4:32:24 PM] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///Users/fbricon/Dev/souk/spring-petclinic/example.xml",
        "version": 13
    },
    "contentChanges": [
        {
            "range": {
                "start": {
                    "line": 7,
                    "character": 22
                },
                "end": {
                    "line": 7,
                    "character": 23
                }
            },
            "rangeLength": 1,
            "text": "\""
        },
        {
            "range": {
                "start": {
                    "line": 7,
                    "character": 14
                },
                "end": {
                    "line": 7,
                    "character": 15
                }
            },
            "rangeLength": 1,
            "text": "\""
        },
        {
            "range": {
                "start": {
                    "line": 5,
                    "character": 32
                },
                "end": {
                    "line": 5,
                    "character": 33
                }
            },
            "rangeLength": 1,
            "text": "\" "
        },
        {
            "range": {
                "start": {
                    "line": 5,
                    "character": 21
                },
                "end": {
                    "line": 5,
                    "character": 22
                }
            },
            "rangeLength": 1,
            "text": "\""
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 32
                },
                "end": {
                    "line": 4,
                    "character": 33
                }
            },
            "rangeLength": 1,
            "text": "\" "
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 21
                },
                "end": {
                    "line": 4,
                    "character": 22
                }
            },
            "rangeLength": 1,
            "text": "\""
        }
    ]
}

[Trace - 4:32:24 PM] Sending notification 'textDocument/didSave'.
Params: {
    "textDocument": {
        "uri": "file:///Users/fbricon/Dev/souk/spring-petclinic/example.xml"
    }
}
angelozerr commented 4 months ago

It seems that there is a but with range formatting. If you select the whole line and you format range you should have the same problem.