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

Add rule YAML Title Alias #256

Merged
merged 21 commits into from
Jul 7, 2022
Merged

Add rule YAML Title Alias #256

merged 21 commits into from
Jul 7, 2022

Conversation

mnaoumov
Copy link
Contributor

@mnaoumov mnaoumov commented Jul 4, 2022

Fixes #254

@pjkaufman
Copy link
Collaborator

@mnaoumov , thabks for creating a PR for this. I just had a couple of questions to make sure I understand how this is supposed to work.

It looks like it forces aliase tags to be multiline arrays no matter what. Am I mistaken or does it actually force the change on the alias key?

How does it handle titles with a colon and a space, a single quote, or a double quote? Is the alias escaped to prevent bad YAML from being created?

Why change the escapeYAMLString method param to an object of type any? Was that because tou were reading in a value from the options?

Overall, it looks pretty good from my once over. I will take a more thorough look later.

@pjkaufman
Copy link
Collaborator

I took a closer look at one of the examples and it looks invalid to me:

Example: Adds a header with the title.

Before:

After:

---
aliases:
  # linter-yaml-title-alias
  - Filename
---

Is there a reason a markdown header is in the frontmatter? This is invalid yaml from my understanding.

@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 4, 2022

@pjkaufman yes, it forces aliases to be a multiline array. It's needed for the reasons mentioned below.

# linter-yaml-title-alias is not a header, it's a YAML comment. It's needed to mark which of the aliases has to be replaced in case if the title changed later. Without this comment we can't distinguish which alias represents old title that has to be replaced vs some other alias that has to be kept.

To denote this functionality, I have two different unit tests: Updates changed alias and Adds title before existing aliases array

Special characters such a colons are handled property and there is a corresponding unit test for that.

escapeYAMLString was changed because it turned out that it can do more than just escaping. Actually it's a YAML serializer. It converts any object into its YAML string representation. In my PR I needed to serialize some object back to the YAML string, that's why I used this function.

@pjkaufman
Copy link
Collaborator

pjkaufman commented Jul 4, 2022

@mnaoumov , is there a reason to not include the yaml comment after the list item like so:

---
aliases:
  - Filename # linter-yaml-title-alias
---

This would allow for attempting to fix any bad yaml that exists which is already a rule (I may need to see about using the YAML formatter as I was unaware escapeYAMLString did that).

Also, are we able to move the forcing of the alias as a multiline array out into a separate rule since it may be undesired that the format of the array changes when using this rule?

@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 4, 2022

@pjkaufman I rewrote my PR in order to preserve existing property styles. Corresponding tests are added

src/rules.ts Outdated Show resolved Hide resolved
src/rules.ts Outdated Show resolved Hide resolved
src/rules.ts Outdated Show resolved Hide resolved
@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 5, 2022

@pjkaufman I refactored my PR to use custom YAML key instead of the comment. It is way more powerful. By keeping the last key, we can quickly detect the no-op case. Also we no longer depend on the order of the aliases, which makes it less fragile

Copy link
Collaborator

@pjkaufman pjkaufman left a comment

Choose a reason for hiding this comment

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

Just a couple of questions before I approve. It looks pretty good. I would personally call the yaml section methods yaml key methods, but that is a minor thing. The only thing I am curious about are the two things I added review comments around.

src/rules.ts Outdated
),
new DropdownOption(
'YAML aliases new array style',
'The style of the aliases YAML property for newly created arrays',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a smaller thing, but could we by chance make it clear that this only applies when going from a single-line string to an array when multiple values are present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am considering to merge two dropdowns into one and making four options

  • Multi-line array
  • Single-line array
  • Single string, that extends to multi-line array if needed
  • Single string, that extends to single-line array if needed

src/test.ts Outdated

const after = dedent`
---
aliases:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it remove the key as well rather than leaving it empty when dealing with a single string?

@pjkaufman
Copy link
Collaborator

Out of curiosity, what happens when you set the multivalue dropdown to single-line array, but your single value dropdown is set to single line? Does it go back to a single-line value when you only have one alias again?

For example

---
aliases:
  - alias 1
  - alias 2
---

has alias 2 removed and becomes:

---
aliases: alias 1
---

@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 7, 2022

Out of curiosity, what happens when you set the multivalue dropdown to single-line array, but your single value dropdown is set to single line? Does it go back to a single-line value when you only have one alias again?

For example

---
aliases:
  - alias 1
  - alias 2
---

has alias 2 removed and becomes:

---
aliases: alias 1
---

We currently don't force any styles for existing aliases sections. The setting is for new sections. If it is already existing, its style is preserved

@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 7, 2022

@pjkaufman I merged the dropdowns to clarify the intent

@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 7, 2022

@pjkaufman I am considering to add a new boolean setting to keep existing style or force the configured one

@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 7, 2022

Just a couple of questions before I approve. It looks pretty good. I would personally call the yaml section methods yaml key methods, but that is a minor thing. The only thing I am curious about are the two things I added review comments around.

I called yaml section because key is just a string, but it can possibly be a large multi-line value, so I though section is the better term for that. Feel free to change the wording if you find it appropriate or consistent

Copy link
Collaborator

@pjkaufman pjkaufman left a comment

Choose a reason for hiding this comment

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

This looks good to me. Did you want to make any more changes before I merge it?

@mnaoumov
Copy link
Contributor Author

mnaoumov commented Jul 7, 2022

I was not thinking about other changes for this particular rule. I have some ideas about global refactoring which I will create a separate issue for

@pjkaufman pjkaufman merged commit 16fc01a into platers:master Jul 7, 2022
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.

Rule: YAML alias from title
2 participants