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

Edit ESF docs added by PR #588 #610

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Conversation

dedemorton
Copy link
Contributor

@dedemorton dedemorton commented Feb 6, 2024

Edits the documentation changes added in #588.

Note that I've tried to make some of the sentences more concise and flow better, but might have inadvertently changed the meaning because the meaning was unclear. Please point out any problems that you find.

I have a few questions for reviewers:

  • What do we mean by "recurring" in the following sentence: "The Elastic Serverless Forwarder ensures at-least-once delivery of the forwarded messages recurring to the Continuing queue and Replay queue." It feels like the wrong word, but I'm not sure what you want to use instead. Maybe just "sent to the Continuing queue...."?
  • In this sentence: "The remaining time is dedicated to sending a copy of the original messages that contains the remaining events to the Continuing queue." Does the verb "contains" apply to the noun "copy" or "messages"? I want to make sure the subject/verb agreement is correct.
  • Minor: Why are we capitalizing "Continuing queue" and "Replay queue"? Does this follow some kind of industry standard or other convention? Also why are these terms italicized everywhere? Typically we only highlight the first occurrence of a term.

What does this PR do?

Ensure that new and changed content adheres to Elastic style, follows English grammar rules, and is easy to understand.

Why is it important?

Good docs make for happy users.

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] I have added an entry in CHANGELOG.md

How to test this PR locally

n/a

Related issues

@dedemorton dedemorton requested a review from aspacca February 6, 2024 23:06
@dedemorton dedemorton self-assigned this Feb 6, 2024
@dedemorton dedemorton requested a review from a team February 6, 2024 23:09
Copy link

github-actions bot commented Feb 6, 2024

File Coverage Lines Branches Missing
All files 100% 100% 100%
main_aws.py 100% 100% 100%
handlers/__init__.py 100% 100% 100%
handlers/aws/__init__.py 100% 100% 100%
handlers/aws/cloudwatch_logs_trigger.py 100% 100% 100%
handlers/aws/exceptions.py 100% 100% 100%
handlers/aws/handler.py 100% 100% 100%
handlers/aws/kinesis_trigger.py 100% 100% 100%
handlers/aws/replay_trigger.py 100% 100% 100%
handlers/aws/s3_sqs_trigger.py 100% 100% 100%
handlers/aws/sqs_trigger.py 100% 100% 100%
handlers/aws/utils.py 100% 100% 100%
share/__init__.py 100% 100% 100%
share/config.py 100% 100% 100%
share/environment.py 100% 100% 100%
share/events.py 100% 100% 100%
share/expand_event_list_from_field.py 100% 100% 100%
share/factory.py 100% 100% 100%
share/include_exlude.py 100% 100% 100%
share/json.py 100% 100% 100%
share/logger.py 100% 100% 100%
share/multiline.py 100% 100% 100%
share/secretsmanager.py 100% 100% 100%
share/utils.py 100% 100% 100%
share/version.py 100% 100% 100%
shippers/__init__.py 100% 100% 100%
shippers/composite.py 100% 100% 100%
shippers/es.py 100% 100% 100%
shippers/factory.py 100% 100% 100%
shippers/logstash.py 100% 100% 100%
shippers/shipper.py 100% 100% 100%
storage/__init__.py 100% 100% 100%
storage/decorator.py 100% 100% 100%
storage/factory.py 100% 100% 100%
storage/payload.py 100% 100% 100%
storage/s3.py 100% 100% 100%
storage/storage.py 100% 100% 100%

Minimum allowed coverage is 100%

Generated by 🐒 cobertura-action against bcc7d64

@aspacca
Copy link
Contributor

aspacca commented Feb 8, 2024

I have a few questions for reviewers:

  • What do we mean by "recurring" in the following sentence: "The Elastic Serverless Forwarder ensures at-least-once delivery of the forwarded messages recurring to the Continuing queue and Replay queue." It feels like the wrong word, but I'm not sure what you want to use instead. Maybe just "sent to the Continuing queue...."?

I'm sorry, I've used what is a false friend related to Italian. "recurring" should be something like "using"

  • In this sentence: "The remaining time is dedicated to sending a copy of the original messages that contains the remaining events to the Continuing queue." Does the verb "contains" apply to the noun "copy" or "messages"? I want to make sure the subject/verb agreement is correct.

The messages contain the remaining events, the copy, being a copy of those message, contains the remaining events as well :)
You're a in a better position than me to choose what's the correct subject/verb agreement :)

  • Minor: Why are we capitalizing "Continuing queue" and "Replay queue"? Does this follow some kind of industry standard or other convention? Also why are these terms italicized everywhere? Typically we only highlight the first occurrence of a term.

I followed that it seemed to me the convention internal to the ESF docs. I might identified the wrong convention.
Please change it as it best, thanks

@dedemorton
Copy link
Contributor Author

dedemorton commented Feb 8, 2024

I followed that it seemed to me the convention internal to the ESF docs.

Yup, that's always the right approach for smaller updates like this one. Generally we only italicize the first instance of a new term. We avoid capitalizing terms unless they are product names (though we have quite a few exceptions). Sometimes we bend the rule if we are discussing a 3rd party technology that follows a different style. This definitely falls into the category of "stuff only editors care about" :-).

Edited: In case it's not clear from my comment, I'll take care of the changes. Just wanted to make sure there wasn't a specific reason why we are following this convention. Thanks!

aspacca
aspacca previously approved these changes Feb 13, 2024
Copy link

@mdbirnstiehl mdbirnstiehl left a comment

Choose a reason for hiding this comment

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

🚀

@dedemorton dedemorton merged commit fd954c6 into elastic:main Feb 13, 2024
5 checks passed
@dedemorton dedemorton deleted the edit-pr-588 branch February 13, 2024 17:54
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