Skip to content

Conversation

mstoykov
Copy link
Contributor

No description provided.

Copy link
Contributor

@MattDodsonEnglish MattDodsonEnglish left a comment

Choose a reason for hiding this comment

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

Just made some suggestions for smoother flow and shorter sentences.
I tried to get to a verb as quickly as possible in each sentence, because a long subject like

Using group with async functions or any kind of asynchronize code

creates a lot of information that the reader must hold in their head before getting to the verb,

does not work.

Copy link
Contributor

@MattDodsonEnglish MattDodsonEnglish left a comment

Choose a reason for hiding this comment

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

@mstoykov Please merge when ready. I'm not sure about when this will come out.

Base automatically changed from linkToTheGroupAPIFromTheGuide to main January 30, 2023 15:19
@mstoykov
Copy link
Contributor Author

force pushed to fix the after merging with rebase the branch this was based on. Also hopefully we will get a preview 🤞

@mstoykov
Copy link
Contributor Author

Comment on lines 20 to 23
<Blockquote mod="warning">

Avoid using `group` with async functions or asynchronous code.
If you do, k6 might apply tags in a way that is unreliable or unintuitive.
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
<Blockquote mod="warning">
Avoid using `group` with async functions or asynchronous code.
If you do, k6 might apply tags in a way that is unreliable or unintuitive.
<Blockquote mod="warning" title="Avoid using group with async operations">
This might cause k6 to apply tags in a way that is unreliable or unintuitive.

| ---- | ------------------------- |
| any | The return value of _fn_. |

<Blockquote mod="warning" title="Working with async functions">
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
<Blockquote mod="warning" title="Working with async functions">
<Blockquote mod="attention" title="Working with async functions">

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize that we need to downgrade the admonition. This is more of a "proceed carefully", not "you will do irreparable harm".

https://developers.google.com/style/notices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest using async and group works so strange (given the input from other k6 OSS contributors), even with changes that are not landing, that it is more or less "if you do this we do not give you any gurantees how it will work". Which kind of borders on "if it will work"

Copy link
Contributor

Choose a reason for hiding this comment

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

But, to quote the style guidelines:

Stronger than a caution notice; it means "Don't do this" or that this step might be irreversible, such as leading to permanent data loss. If a reader doesn't heed the warning, they can lose money, lose work, or open themselves to a security breach. For example, "Don't put a password on the command line; doing so is a security risk."

I mean, if async is this bad in groups, is it possible to just forbid completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do that to the extend that is possible and then this is here to both note that this is the case, link to the issue that explains this in more details and tell people to not even try to circument this as it will likely not work as they think it will.

@mstoykov mstoykov requested a review from na-- January 31, 2023 08:23
@mstoykov mstoykov merged commit 1471633 into main Jan 31, 2023
@mstoykov mstoykov deleted the asyncGroupUpdate branch January 31, 2023 09:03
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.

3 participants