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

Readme improvements #258

Open
wants to merge 6 commits into
base: 2.x
Choose a base branch
from
Open

Readme improvements #258

wants to merge 6 commits into from

Conversation

gforcada
Copy link
Member

@gforcada gforcada commented Feb 14, 2025

Very minimal formatting changes, check each commit separately to see each change in isolation.

@gforcada gforcada force-pushed the readme-improvements branch from 0370283 to c0b814f Compare February 15, 2025 16:06
@gforcada gforcada force-pushed the readme-improvements branch from c0b814f to 6621b68 Compare February 15, 2025 17:38
@gforcada gforcada requested a review from stevepiercy February 15, 2025 18:03
@gforcada gforcada mentioned this pull request Feb 15, 2025
stevepiercy added a commit to plone/documentation that referenced this pull request Feb 16, 2025
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

See related PR in Documentation.

plone/documentation#1863

See comments regarding:

  • One sentence per line
  • It's hard for me to review this PR due to all the white space noisy diffs, which I assume is due to VSCode needlessly reformatting it. I'll push .vscode/settings.json to avoid that in the future, but it would sure be nice not to have to pick through all that noise because I may have missed something critical as a result of the noise. (Actually, this should be a default file in all Plone projects, but that's for another PR.)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
### Manage configuration files

For each of the configuration files, you should edit its [corresponding stanza](#applying-a-customized-configuration) in the file `.meta.toml`.
To customize how the managed files are created,
set their options on `.meta.toml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

They are not always created, but overwritten. This is not correct. Please revert and append the phrase to the end of the original sentence to customize them.

This is also an example of why we should use one sentence per line in narrative documentation. During a review, when I want to make a suggestion, I have to make multiple suggestions, one per line. I'm lazy. Instead I put a single comment, which means you won't have the buttons to Commit suggestion or Add suggestion to batch. [Note to self: I should add that to the Documentation style guide.]

Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored this sentence because the [corresponding stanza](#xxx) was linking to a non-existing header.

They are not always created, but overwritten. This is not correct.

Well, yes, they are created and managed by plone.meta. To change how they are rendered you need to add entries on .meta.toml.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gforcada
Copy link
Member Author

@stevepiercy thanks for all the reviews, I will have a look.

As for the reformatting: I have a markdown linter that was screaming all over, so it was not VSCode autoformatting.

As for the noise: I usually spend (too much 😓 ) time reviewing and refactoring my commits to make them isolated and easier to review, so all the noise you mention is kept in a single commit. If you use GitHub's commit browser you will be able to see the changes much more easily.

@stevepiercy
Copy link
Contributor

As for the reformatting: I have a markdown linter that was screaming all over, so it was not VSCode autoformatting.

Can you turn it off?

As for the noise: I usually spend (too much 😓 ) time reviewing and refactoring my commits to make them isolated and easier to review, so all the noise you mention is kept in a single commit. If you use GitHub's commit browser you will be able to see the changes much more easily.

How can I do that and do a review in GitHub? AFAIK, the review interface compares the initial state against the current state after all commits.

stevepiercy added a commit to plone/documentation that referenced this pull request Feb 17, 2025
Merge only after plone/meta#258 is merged.
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I'd prefer to use 4-space indentation in .md files. It's consistent with Python, code-block, makes enumerated and bulleted lists consistent (3 vs. 2 spaces), and improves readability for authors. Linters for .md trample all over that. I didn't review any reformatted content.

Comment on lines +61 to +62
To customize how the managed files are created,
set their options on `.meta.toml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To customize how the managed files are created,
set their options on `.meta.toml` file.
For each of the configuration files, you should edit its [corresponding stanza](#config-package-configuration-files) in the file `.meta.toml` to customize them.

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.

2 participants