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

chore: fixup yaml front matter toward more widely parsable standard #512

Closed

Conversation

philoserf
Copy link
Contributor

@philoserf philoserf commented Oct 8, 2022

Edited

  • Updated YAML from matter in markdown files
    - the update correctly handles an empty array
    - quotes special characters in strings
    - including strings with embedded strings
  • Some incidental whitespace changes like trailing whitespace

Added

  • No new content here

Checklist

@claremacrae
Copy link
Contributor

Thank you for the contribution!

Reviewing the changes - how they were made

I tried reviewing it in Safari on my Mac, but there are so many changes that the browser was so slow as to be unusable.

Please can you tell us a bit more about how you made the change (for example, manually, or with some tool - which one?), and the benefits of it? (What's the significance of the YAML being parsable, for example).

I ask because if they were entirely made by automated tools, then review can be quite quick. But if there were any manual edits at all, then reviewing will be time-consuming.

It's maybe too late now, but personally, I would have preferred the YAML changes to have been made as a separate PR from the markdown-formatting changes, for ease of reviewing.

The current markdownlint mechanism

Also, how does it relate to the current markdownlint mechanism that we have?
https://publish.obsidian.md/hub/00+-+Contribute+to+the+Obsidian+Hub/03+Contributor+Notes/03.03+Scripts+and+Automation/Checking+Hub+Content

That runs this script. Note the comments there about checks that we would like to enable.
https://github.com/obsidian-community/obsidian-hub/blob/main/.github/scripts/run_markdownlint.sh

That uses the related two files:
https://github.com/obsidian-community/obsidian-hub/blob/main/.markdownlintignore
https://github.com/obsidian-community/obsidian-hub/blob/main/.markdownlintrc

Updating the markdownlint config

It seems like this change is adding some new expectations to the formatting of the markdown, like no trailing spaces perhaps.

I would like to see how the new markdown formatting relates to any of the markdownlint rules? Do they enable any additional rules to be turned on - or currently disabled ones to be un-disabled?

I do wonder whether the Markdown formatting aspects of this change might be better (as in, more easily reviewed, and clearer what the changes are) done by making the markdownlint rules stricter one by one, and getting that tool to re-format the markdown - and committing at each step.

I would be happy to help with that process...

@claremacrae
Copy link
Contributor

The python tests will need reviewing:
https://github.com/obsidian-community/obsidian-hub/actions/runs/3212189491/jobs/5252961300#step:5:51

I can't tell immediately from the output whether it's simply a matter of updating expected output from the tests, or whether the tests have found an issue in one of the new templates.

@claremacrae
Copy link
Contributor

Being reminded of the Python tests reminded me that we have another set of templates that are used with Jinja2 for machine-generating new pages every week:

https://github.com/obsidian-community/obsidian-hub/tree/main/.github/scripts/templates

The formatting in those templates will also need to be updated to match the new formatting in this PR, so that new generated pages are the expected layout.

@claremacrae
Copy link
Contributor

@philoserf Would you consider one or two remote pairing sessions with me, so that we can achieve the goals of this PR in an incremental way that fits in with the existing automation in this repo please?

@philoserf
Copy link
Contributor Author

philoserf commented Oct 9, 2022

I approached discovering, correcting, and offering these changes a few ways since I first opened the Hub.

TL;DR: These were made with tooling and some hand edits where the tooling could not handle the YAML problem.

History: (Caveat; All methods tested the computer and tool I was using)

  • I explored just fixing the YAML tags and aliases arrays with a regex search and replace in Visual Studio Code
  • I explored configuring prettier to do the job
  • I eventually found the linter Obsidian plugin and, using only the YAML tab, could handle most cases. There were a few cases that Linter couldn't handle, and I edited those by hand. These were mainly the @name or the "[[stuff "other stuff"]]" kinda fixes. That had the side effect of "fixing" stuff my editor just "fixes" for me, like trailing spaces.
  • As I review this pull request with a reviewer's eye, I see that I can offer minor updates and introduce fewer changes with an incremental linter plugin config and one type of change at a time. That would take longer as most would depend upon the previous. Do-able. Also, I wasn't concerned with the markdown changes when I started. I can remove those "incidental" changes with better use of git add -p to split and leave those behind.

Finally, I would be happy to pair on the problem. I wasn't sure the project cared or wanted this kind of change. Many don't. I'm just a wee bit of a formatting/linting obsessive.

P.S. I will review the markdown lint/config mentioned above at look at the python tests too.

ref: https://github.com/platers/obsidian-linter

@philoserf
Copy link
Contributor Author

I will close this pull request in favor of whatever we decide the incremental approach shall be.

@philoserf philoserf closed this Oct 9, 2022
@philoserf philoserf deleted the cleanup-markdown-yaml branch October 9, 2022 17:19
@claremacrae
Copy link
Contributor

claremacrae commented Oct 10, 2022

Hi @philoserf, Many thanks for your understanding...

I haven't used the Linter plugin. If we could set something up so that people get the identical output via the Linter as they do with the linter configuration at the root of this vault, that would be a big win for contributors, I feel.

I've sent you an email separately to see if we can set up a pairing session.

Thanks again!

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.

None yet

2 participants