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.

Sign up for GitHub

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

Jump to bottom

Embrace that help strings are markdown. #12606

Merged
merged 2 commits into from
Aug 19, 2021
Merged

Embrace that help strings are markdown. #12606

merged 2 commits into from
Aug 19, 2021

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Aug 19, 2021

Previously we sometimes wanted help strings treated as markdown and sometimes as plain text,
depending on author convenience. Or we wanted parts of a given string treated in different ways.
We tried various hacks to anticipate author intention, but they never worked properly, and caused
no end of errors and edge cases and gotchas.

Now we simply accept that help strings are treated as markdown. This makes sense: We need
them to be in a format that is human-readable (for when displaying help on the CLI, or when reading
source code), and also machine-readable, for rendering by readme.com. Markdown is such a
dual format.

This change:

  • Removes the weird heuristics.
  • Fixes up various help strings to conform with the new reality.

The big gotcha now is that authors of help strings must be aware of this fact. For example, you will
often want to surround things that look like markup or HTML tags, but are not, with backticks.

I have manually eyeballed a large subset of the docs generated with these changes, including
the "known offenders", and this seems like a good way to go.

A possible future change is to detect when help strings have changed, render them, and prompt
the author to manually verify that the rendered pages look good.

[ci skip-rust]

[ci skip-build-wheels]

@benjyw
Embrace that help strings are markdown.
4a389b9 
[ci skip-rust]

[ci skip-build-wheels]
@benjyw benjyw requested a review from Eric-Arellano August 19, 2021 18:28
@benjyw
Copy link
Contributor Author

benjyw commented Aug 19, 2021

@njgrisafi this should address issues like the one you found!

@Eric-Arellano
Copy link
Contributor

A possible future change is to detect when help strings have changed, render them, and prompt
the author to manually verify that the rendered pages look good.

That'd be sweet. Might be worth having as a TODO

Eric-Arellano
Eric-Arellano approved these changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Sgtm! I prefer help messages always being markdown even on the CLI.

@benjyw
Fix test
58c78b4 
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@benjyw benjyw merged commit c9bb31e into pantsbuild:main Aug 19, 2021
@benjyw benjyw deleted the markdown_help branch August 19, 2021 20:37
benjyw added a commit to benjyw/pants that referenced this pull request Aug 19, 2021
@benjyw
Embrace that help strings are markdown. (pantsbuild#12606)
17c0ec1 
Previously we sometimes wanted help strings treated as markdown and sometimes as plain text,
depending on author convenience. Or we wanted parts of a given string treated in different ways.
We tried various hacks to anticipate author intention, but they never worked properly, and caused
no end of errors and edge cases and gotchas.

Now we simply accept that help strings are treated as markdown. This makes sense: We need
them to be in a format that is human-readable (for when displaying help on the CLI, or when reading
source code), and also machine-readable, for rendering by readme.com. Markdown is such a
dual format.

This change:

Removes the weird heuristics.
Fixes up various help strings to conform with the new reality.
The big gotcha now is that authors of help strings must be aware of this fact. For example, you will
often want to surround things that look like markup or HTML tags, but are not, with backticks.

I have manually eyeballed a large subset of the docs generated with these changes, including
the "known offenders", and this seems like a good way to go.

A possible future change is to detect when help strings have changed, render them, and prompt
the author to manually verify that the rendered pages look good.

[ci skip-rust]

[ci skip-build-wheels]
benjyw added a commit that referenced this pull request Aug 19, 2021
@benjyw
Embrace that help strings are markdown. (#12606) (#12608)
96f7793 
Previously we sometimes wanted help strings treated as markdown and sometimes as plain text,
depending on author convenience. Or we wanted parts of a given string treated in different ways.
We tried various hacks to anticipate author intention, but they never worked properly, and caused
no end of errors and edge cases and gotchas.

Now we simply accept that help strings are treated as markdown. This makes sense: We need
them to be in a format that is human-readable (for when displaying help on the CLI, or when reading
source code), and also machine-readable, for rendering by readme.com. Markdown is such a
dual format.

This change:

Removes the weird heuristics.
Fixes up various help strings to conform with the new reality.
The big gotcha now is that authors of help strings must be aware of this fact. For example, you will
often want to surround things that look like markup or HTML tags, but are not, with backticks.

I have manually eyeballed a large subset of the docs generated with these changes, including
the "known offenders", and this seems like a good way to go.

A possible future change is to detect when help strings have changed, render them, and prompt
the author to manually verify that the rendered pages look good.

[ci skip-rust]

[ci skip-build-wheels]
@illicitonion illicitonion mentioned this pull request Aug 30, 2021
Merged
illicitonion added a commit to illicitonion/pants that referenced this pull request Aug 30, 2021
@illicitonion
Prep for 2.7.0rc0
2115af7 
* Clean up the config parsing code. ([pantsbuild#12678](pantsbuild#12678))

* [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([pantsbuild#12692](pantsbuild#12692))

* [internal] Add missing `resources` targets for default tool lockfiles ([pantsbuild#12670](pantsbuild#12670))

* [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([pantsbuild#12669](pantsbuild#12669))

* [internal] Test that default tool lockfiles work on macOS ([pantsbuild#12666](pantsbuild#12666))

* [internal] Improve the error message for stale/invalid lockfiles ([pantsbuild#12618](pantsbuild#12618))

* [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([pantsbuild#12641](pantsbuild#12641))

* [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([pantsbuild#12665](pantsbuild#12665))

* [internal] Test that Python tools work with all expected interpreter versions ([pantsbuild#12656](pantsbuild#12656))

* [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([pantsbuild#12630](pantsbuild#12630))

* [internal] Add links to changelog and Twitter in Pants PyPI project page. ([pantsbuild#12636](pantsbuild#12636))

* [internal] Update CoC with proper markdown + https links. ([pantsbuild#12637](pantsbuild#12637))

* GitHub issue templates ([pantsbuild#12617](pantsbuild#12617))

* Register subsystems and target types by backend/plugin. ([pantsbuild#12623](pantsbuild#12623))

* [internal] Sort input requirements for `LockfileMetadata` ([pantsbuild#12615](pantsbuild#12615))

* [internal] Expect lockfile metadata to be defined ([pantsbuild#12616](pantsbuild#12616))

* Simplify subsystem registration. ([pantsbuild#12620](pantsbuild#12620))

* [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([pantsbuild#12613](pantsbuild#12613))

* [internal] Refactor `lockfile_metadata.py` to be more class-based ([pantsbuild#12611](pantsbuild#12611))

* Embrace that help strings are markdown. ([pantsbuild#12606](pantsbuild#12606))

* Stop fetching a lockfile request just to figure out the requirements digest ([pantsbuild#12607](pantsbuild#12607))

* [internal] Refactor MyPy first party plugins ([pantsbuild#12599](pantsbuild#12599))

* [internal] Refactor MyPy config file setup ([pantsbuild#12593](pantsbuild#12593))

* [internal] use `is_union()` rather than `union.is_instance()`. ([pantsbuild#12595](pantsbuild#12595))

* fs_util can use mTLS and ignore certificates ([pantsbuild#12586](pantsbuild#12586))

* [internal] Fix Pylint lockfile to actually be wired up ([pantsbuild#12590](pantsbuild#12590))

* [internal] Update lockfile TODOs with current state of project ([pantsbuild#12592](pantsbuild#12592))

* [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints  ([pantsbuild#12566](pantsbuild#12566))

* [internal] Refactor Pylint first-party plugins ([pantsbuild#12583](pantsbuild#12583))

* [internal] Fix Pytest and IPython to check for stale lockfiles ([pantsbuild#12582](pantsbuild#12582))

* [internal] Do not cache lockfile generation ([pantsbuild#12674](pantsbuild#12674))
illicitonion added a commit that referenced this pull request Aug 30, 2021
@illicitonion
Prep for 2.7.0rc0 (#12695)
b86d463 
* Clean up the config parsing code. ([#12678](#12678))

* [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([#12692](#12692))

* [internal] Add missing `resources` targets for default tool lockfiles ([#12670](#12670))

* [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([#12669](#12669))

* [internal] Test that default tool lockfiles work on macOS ([#12666](#12666))

* [internal] Improve the error message for stale/invalid lockfiles ([#12618](#12618))

* [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([#12641](#12641))

* [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([#12665](#12665))

* [internal] Test that Python tools work with all expected interpreter versions ([#12656](#12656))

* [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([#12630](#12630))

* [internal] Add links to changelog and Twitter in Pants PyPI project page. ([#12636](#12636))

* [internal] Update CoC with proper markdown + https links. ([#12637](#12637))

* GitHub issue templates ([#12617](#12617))

* Register subsystems and target types by backend/plugin. ([#12623](#12623))

* [internal] Sort input requirements for `LockfileMetadata` ([#12615](#12615))

* [internal] Expect lockfile metadata to be defined ([#12616](#12616))

* Simplify subsystem registration. ([#12620](#12620))

* [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([#12613](#12613))

* [internal] Refactor `lockfile_metadata.py` to be more class-based ([#12611](#12611))

* Embrace that help strings are markdown. ([#12606](#12606))

* Stop fetching a lockfile request just to figure out the requirements digest ([#12607](#12607))

* [internal] Refactor MyPy first party plugins ([#12599](#12599))

* [internal] Refactor MyPy config file setup ([#12593](#12593))

* [internal] use `is_union()` rather than `union.is_instance()`. ([#12595](#12595))

* fs_util can use mTLS and ignore certificates ([#12586](#12586))

* [internal] Fix Pylint lockfile to actually be wired up ([#12590](#12590))

* [internal] Update lockfile TODOs with current state of project ([#12592](#12592))

* [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints  ([#12566](#12566))

* [internal] Refactor Pylint first-party plugins ([#12583](#12583))

* [internal] Fix Pytest and IPython to check for stale lockfiles ([#12582](#12582))

* [internal] Do not cache lockfile generation ([#12674](#12674))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eric-Arellano Eric-Arellano Eric-Arellano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benjyw @Eric-Arellano