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

Update Colcon-Tutorial.rst #4867

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

Update Colcon-Tutorial.rst #4867

wants to merge 1 commit into from

Conversation

M61ss
Copy link
Contributor

@M61ss M61ss commented Nov 14, 2024

clarification

clarification

Signed-off-by: Mattia Massarenti <[email protected]>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Personally I think this is very subjective; the sentence as-is is correct, and I don't find that this change improves the readability. Because of that, I'm inclined not to take this.

But I'll leave it open for a little while longer to see if one of the other maintainers disagrees.

@christophebedard
Copy link
Member

Personally, I'm a fan of using a comma in this case, although the "then" is just extra. I would vote in favour of this change, but I understand that this is kind of opening the door to a ton of tiny, sometime subjective (depending on grammatical preferences, and sometimes native languages) changes like this one.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

i am okay to take this 😃

@M61ss
Copy link
Contributor Author

M61ss commented Nov 15, 2024

I can remove the "then" as I agree that it sounds heavy. The reason of my pull request is that reading the documentation I found difficult to understand the period, I had to read it many times. I am not a motherlanguage, but I am pretty sure that condition and consequence should be separated by a comma to be more readable. Sincerely, I added the "then" because it was embarrassing for me to open a pull request only for a comma.
Anyway, I didn't want to open a controversy about a comma. Feel free to do whatever you want with this doc, I only provided my opinion.

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

It's not really controversial. This kind of change may just not be seen as "critical" enough for some people, and, given that there are a lot more sentences like this, it would be a bit of work if we were to change all of them. However, you're obviously only proposing this one change, so I think it's fine.

It does indeed sound a bit weird to me as someone who is not a native English speaker and instead speaks a language that tends to use commas a lot more than the English language does. So again I'd vote to take this.

@M61ss
Copy link
Contributor Author

M61ss commented Nov 15, 2024

Ok, thank you for your suggestion.

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.

4 participants