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] Some consistency changes to env variables in custom-commands.md #4998

Merged

Conversation

rpkoller
Copy link
Collaborator

@rpkoller rpkoller commented Jun 17, 2023

While adding DDEV_UID and DDEV_GID in a PR i've noticed that the env variable descriptions are sort of inconsistent. For one sometimes the first word after the colon is in lower case and sometimes in upper case. I thought after colon it should always be upper case? the only exception i've made for the description of DDEV_PROJECT_TYPE that just lists the cms versions which are names - same for DDEV_WEBSERVER_TYPE. And the other detail i wonder but wasn't sure in the end. would it make sense to end each description with a period cuz some already are? cuz the descriptions aren't always full sentences?

The Issue

How This PR Solves The Issue

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

While adding DDEV_UID and DDEV_GID in a PR i've noticed that the env variable descriptions are sort of inconsistent. For one sometimes the first word after the colon is in lower case and sometimes in upper case. I thought after colon it should always be upper case? the only exception i've made for the description of DDEV_PROJECT_TYPE that just lists the cms versions which are names - same for DDEV_WEBSERVER_TYPE. And the other detail i wonder but wasn't sure in the end. would it make sense to end each description with a period cuz some already are? cuz the descriptions aren't always full sentences?
@rpkoller rpkoller requested a review from a team as a code owner June 17, 2023 14:04
Copy link
Member

@gilbertsoft gilbertsoft left a comment

Choose a reason for hiding this comment

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

Thanks! Please rebase your branch, looks like you did not use the latest master.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks!

@rfay rfay changed the title Some consistency changes to env variables in custom-commands.md [docs] Some consistency changes to env variables in custom-commands.md Jun 18, 2023
@rpkoller
Copy link
Collaborator Author

resolved the conflict in this PR as well. the only question that is still open if there should be periods at the end of each description or if it should be left as is and just the change to upper case after periods should be done?

@gilbertsoft
Copy link
Member

resolved the conflict in this PR as well. the only question that is still open if there should be periods at the end of each description or if it should be left as is and just the change to upper case after periods should be done?

Thanks! We are able to merge now, conflicts resolved :)

Imho no periods needed here but maybe a native English has another opinion.... @mattstein ?

@rpkoller
Copy link
Collaborator Author

oh I've just noticed, on my initial PR on some descriptions there were periods at the end and on some there were not. after the rebase there aren't any periods anymore only for the two descriptions with etc at the end, but there the period is for the abbreviation etc. and since as already mentioned in the issue summary the descriptions aren't always full sentences i would strike my suggestion about the periods.

@mattstein
Copy link
Sponsor Collaborator

Imho no periods needed here but maybe a native English has another opinion.... @mattstein ?

I agree. These aren’t entire sentences so ending punctuation isn’t necessary.

@rfay rfay merged commit 66833de into ddev:master Jun 20, 2023
8 checks passed
@rpkoller rpkoller deleted the 20230617-consistency-changes-env-descriptions branch August 24, 2023 11:37
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

4 participants