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

docs: move make build instruction from paragraph into list #3193

Merged
merged 3 commits into from Mar 21, 2023

Conversation

jrpear
Copy link
Contributor

@jrpear jrpear commented Mar 21, 2023

Description

Move make build instruction from paragraph into list

Clarifies confusion in #3191

Type of change

  • This change requires a documentation update

@jrpear
Copy link
Contributor Author

jrpear commented Mar 21, 2023

The diff for this change is a bit ugly because I had to renumber the items in the list.

Markdown doesn't actually care what the number before the period is. These two lists render the same:

1. First, build the container image: `make build`. You can then:
2. Run all tests: `make clean tests`
3. Run a single test: `make clean generate-accounts test/<TEST NAME WITHOUT .bats SUFFIX>`
4. Run multiple unrelated tests: `make clean generate-accounts test/<TEST NAME WITHOUT .bats SUFFIX>,<TEST NAME WITHOUT .bats SUFFIX>` (just add a `,` and then immediately write the new test name)
5. Run a whole set or all serial tests: `make clean generate-accounts tests/parallel/setX` where `X` is the number of the set or `make clean generate-accounts tests/serial`

and

1. First, build the container image: `make build`. You can then:
1. Run all tests: `make clean tests`
1. Run a single test: `make clean generate-accounts test/<TEST NAME WITHOUT .bats SUFFIX>`
1. Run multiple unrelated tests: `make clean generate-accounts test/<TEST NAME WITHOUT .bats SUFFIX>,<TEST NAME WITHOUT .bats SUFFIX>` (just add a `,` and then immediately write the new test name)
1. Run a whole set or all serial tests: `make clean generate-accounts tests/parallel/setX` where `X` is the number of the set or `make clean generate-accounts tests/serial`

The latter makes for nicer diffs and editing. What do you think of changing every list in the docs to use the latter sort of list?

I suppose the downside is that the raw markdown is marginally less nice.

2. Run a single test: `make clean generate-accounts test/<TEST NAME WITHOUT .bats SUFFIX>`
3. Run multiple unrelated tests: `make clean generate-accounts test/<TEST NAME WITHOUT .bats SUFFIX>,<TEST NAME WITHOUT .bats SUFFIX>` (just add a `,` and then immediately write the new test name)
4. Run a whole set or all serial tests: `make clean generate-accounts tests/parallel/setX` where `X` is the number of the set or `make clean generate-accounts tests/serial`
1. First, build the container image: `make build`. You can then:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. First, build the container image: `make build`. You can then:
1. Run `make build` to create or update the local `mailserver-testing:ci` Docker image (_using the projects `Dockerfile`_)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Run `make build` to create or update the local `mailserver-testing:ci` Docker image (_using the projects `Dockerfile`_)

doesn't say that the following commands depend on the first one. I think this might be better:

First, run `make build` to create or update the local `mailserver-testing:ci` Docker image (_using the projects `Dockerfile`_)

Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Thanks again! 😀

@polarathene polarathene added area/tests kind/improvement Improve an existing feature, configuration file or the documentation area/documentation labels Mar 21, 2023
@polarathene polarathene added this to the v12.0.0 milestone Mar 21, 2023
@github-actions
Copy link
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: 7417025

@polarathene polarathene merged commit b3249fa into docker-mailserver:master Mar 21, 2023
2 checks passed
@polarathene
Copy link
Member

The latter makes for nicer diffs and editing. What do you think of changing every list in the docs to use the latter sort of list?

My preference is to keep the order explicit, we rarely need to make such changes. Diff quality in Github web UI isn't great but I think some other tools do a better job (eg: Meld) usually at rendering the relevant changes.

I also know that our docs generator mkdocs has some GFM compatibility issues. This is also a problem with formatters like Prettier which would break this part of the docs (I raised an issue upstream about it, but nothing has improved on that front AFAIK).

IIRC, one of those is that indented/nested lists need to be 4 spaces wide, anything less and it's treated at the same level, and increments the items with the same logic that you bring up.

@jrpear
Copy link
Contributor Author

jrpear commented Mar 21, 2023

My preference is to keep the order explicit, we rarely need to make such changes. Diff quality in Github web UI isn't great but I think some other tools do a better job (eg: Meld) usually at rendering the relevant changes.

Ah makes sense. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation area/tests kind/improvement Improve an existing feature, configuration file or the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants