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

Introduce yaml formatting #74

Open
wants to merge 11 commits into
base: v0.21
Choose a base branch
from
Open

Introduce yaml formatting #74

wants to merge 11 commits into from

Conversation

nnmrts
Copy link
Contributor

@nnmrts nnmrts commented Nov 20, 2024

For easier and more efficient editing, I propose adding a .vscode/settings.json file that enables autoformatting YAML (in VSCode at least). I also included the autoformatted masterlist.yaml in this PR.

Sadly, there is seemingly no VSCode extension that allows configuration to format YAML to precisely follow the guide at https://loot.github.io/docs/contributing/Masterlist-Editing:

Arbitrary string values should be enclosed in single quotes. If the string contains any single quotes, they should be repeated.

The part I emphasized from the quote above is handled differently by the most common yaml formatters in VSCode. A single quote string containing '' to escape ' is not kept as is, unless that same string also includes ". Instead the whole string is converted to a double quoted string in that instance (an example of that behavior can be seen in the formatted masterlist.yaml included in this PR.

At several places I've read that single quote and double quote strings can actually mean different things in YAML, but I'm not sure if that is relevant in this context and if some double quoted strings would break any YAML parsing done by LOOT or other modding community tools. Replacing ' in strings everywhere with "proper" apostrophes () as a workaround wouldn't be a feasible option because file name strings need to be exact and could include ' meaning either apostrophe or single quote.

@nnmrts
Copy link
Contributor Author

nnmrts commented Nov 20, 2024

Also, in case this gets accepted, before merging it should probably be discussed at a central LOOT place like https://github.com/loot/loot or the LOOT Discord server, especially because https://loot.github.io/docs/contributing/Masterlist-Editing needs to be updated then.

@nnmrts nnmrts changed the title Intoduce yaml formatting Introduce yaml formatting Nov 20, 2024
@Ortham
Copy link
Member

Ortham commented Nov 20, 2024

I'd be fine with changing the formatting to use double quotes instead of single quotes when any escaping is needed if it means auto-formatting would be easier.

At several places I've read that single quote and double quote strings can actually mean different things in YAML

AFAIK they mean the same thing but there are strings that you can express in double quotes that can't be expressed in single quotes. That does mean that parsing double-quoted strings is more difficult, but I don't think that really scratches the surface of what makes parsing YAML famously horrible.

@pStyl3
Copy link
Member

pStyl3 commented Nov 20, 2024

I too would be fine with changing the formatting to use double quotes instead of single quotes.

However, I don't think that it would be necessary to change the indentation of # comments or to remove empty spaces, e.g. changing subs: [ 'someurl' ] to subs: ['someurl'] reduces readability in my opinion.

@nnmrts
Copy link
Contributor Author

nnmrts commented Nov 21, 2024

I don't think that it would be necessary to change the indentation of # comments

Unfortunately I couldn't find a way to avoid this with the current yaml formatter.

or to remove empty spaces

See 1ea16ba, which I hoped would fix this but it didn't.

I'd be fine with changing the formatting to use double quotes

I too would be fine with changing the formatting to use double quotes

Nice, I also prefer double quotes. See 5ac47bc.

However, that's also not perfect. Condition strings, which of course often include double quotes themselves, and single quoted file strings with regex patterns are seemingly kept as is. One way or another, it seems to be the case that we have to be okay with mixed quoted strings.

What's worse in my opinion: the formatting isn't "deterministic" or rather "input-agnostic".

Take this string for example:

  - name: "legendaries\\.es[mp]"

Whatever "yaml.format.singleQuote" is set to, the formatter keeps this string as is. However, if the string looks like this:

  - name: 'legendaries\.es[mp]'

"yaml.format.singleQuote" also makes no difference and the string is kept as is.

This could obviously lead to both styles being present in the final file and yet again, manual review of the format is necessary.

Unless you're okay with that, I'll look into other yaml formatters, but after my recent research about them, I think the situation, at least regarding JS-based yaml formatters, is dire.

@nnmrts
Copy link
Contributor Author

nnmrts commented Nov 22, 2024

In my latest commit, I switched to the extension "Better YAML Formatter", which reintroduced unconfigurable single quotes but fixes this:

changing subs: [ 'someurl' ] to subs: ['someurl'] reduces readability in my opinion

I tried several different and increasingly obscure options but I think this is the best I can do without writing a not insane yaml formatter myself.

@Ortham
Copy link
Member

Ortham commented Dec 4, 2024

Sorry for the delayed reply, but I've just taken a look at the latest formatting, and I'm happy with it. Thinking about how to explain it in the style guide, would it be accurate to say something like the following?

If it's an arbitrary string that doesn't contain any single quotes or any characters that need to be written as escape sequences, enclose it in single quotes. If it's an arbitrary string that contains single quotes or characters that need to be written as escape sequences, enclose it in double quotes and escape anything that needs to be escaped (including any double quotes).

@nnmrts
Copy link
Contributor Author

nnmrts commented Dec 10, 2024

Sorry for the delay as well, GitHub's notification system seriously got some explaining to do here...

@Ortham Yes, that makes sense and looks good to me, but obviously the autoformatter can't always follow these rules as I said. I don't know if that's a big deal and/or people should be aware of it. Might be useful as a side fact somewhere on that same page or even as a second sentence; could even link to this issue here.

@pStyl3
Copy link
Member

pStyl3 commented Mar 1, 2025

@nnmrts Could you rebase your PR? There are some minor conflicts right now.

That being said, while @Ortham said that he's happy with the formatting, right now I can't say so for myself. The change in how this PR indents comments still seems arbitrary to me.

I'd like to see this change in action for myself, so I've created a small .yaml file to test this with - see Discord.

image

Can you guide me through in how to actually apply your proposed changes?

@nnmrts
Copy link
Contributor Author

nnmrts commented Mar 1, 2025

@pStyl3 Sorry for not reacting on Discord, haven't opened it for a while. 😝

Seems like "Better YAML Formatter" doesn't quite work anymore or I'm doing something wrong. I've reverted to "redhat.vscode-yaml" in the latest commit, of course this reintroduces the issue I referenced fixing at #74 (comment) but enables double quotes.

Can you try again? It should just be as easy as opening the yaml file in VSCode and saving it. I added "editor.formatOnSave": true to the settings.json for now in case you don't have that setting enabled in your local settings, but that should ideally be not included in the final PR.

Regarding the comment indenting: This is one of the things out of my or seemingly anyone's control. As I've previously hinted at, the world of YAML formatters feels more desolated than Starfield's planets.

@pStyl3
Copy link
Member

pStyl3 commented Mar 2, 2025

@pStyl3 Sorry for not reacting on Discord, haven't opened it for a while. 😝

No problem at all, I'll appreciate your effort to create an inprovement for the masterlists.

Seems like "Better YAML Formatter" doesn't quite work anymore or I'm doing something wrong.

With your added editor.formatOnSave line I was able to make kennylongs YAML formatter work. Here's the result of that:
yamltest_v1.zip

At least with kennylong's YAML formatter, the changes (with your proposed settings.json, see here) are not only about comments, if there are multiple empty lines after another, only one remaines after the auto-formatting. The amount of spaces plugin entries get indented also changes, from

plugins:
###### Official Game Files ######
  - name: 'Skyrim.esm'
    group: *mainGroup
  - name: 'Update.esm'
    group: *mainGroup

to

plugins:
###### Official Game Files ######
- name: 'Skyrim.esm'
  group: *mainGroup
- name: 'Update.esm'
  group: *mainGroup

Another example from the prelude node within the masterlist, it goes from

prelude:
  common:
  # Message Anchors
    - &alreadyInX
      type: error
      content: 'Delete. Already included in {0}.'

to

prelude:
  common:
  # Message Anchors
  - &alreadyInX
    type: error
    content: 'Delete. Already included in {0}.'

If we would get kennylong's YAML formatter to work in the sense that it successfully changes single quotes '' to double quotes "", without changing the other stuff, it might be a possible solution. Like it is right now, I'm afraid I'm not to keen on this solution.

I've reverted to "redhat.vscode-yaml" in the latest commit, of course this reintroduces the issue I referenced fixing at #74 (comment) but enables double quotes.

Can you try again?

I've also tried to use redhat's YAML formatter, but so far with no success. I've changed the settings.json to this version and installed the following extension in VSCode:

image

I'm assuming this is the right one? But as said, so far I couldn't get it to work. The outcome that can be seen in the commit linked above doesn't seem to be as "invasive", which seems better..

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.

3 participants