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(parser): change parser documentation #5262

Merged
merged 20 commits into from
Nov 7, 2024

Conversation

anafalcao
Copy link
Collaborator

@anafalcao anafalcao commented Sep 27, 2024

Issue number: #1371

Summary

This PR refreshes Parser docs with newer features like examples, improves wording and navigation, focused on Powertools v3.

Changes

  • Clean up the initial explanation
  • Clean up Key Features
  • Move the info "this is not necessary if using Layers" to the top
  • Remove Pydantic 1 integration
  • Add a QuickStart with BaseModels and Parse events
  • Add sections Getting Started and Advanced
  • Fix the parse() function example and initial explanation
  • Add differences between of parse() and event_parser
  • Clean up the example of extending built-in models; add pre and post explanations
  • Clean up the Envelopes example and improve the initial explanation
  • Complete the Validating fields example
  • Clean up the root_validator example
  • Move the Pydantic helper functions box to Getting started section
  • Restructure Serialization; add a functional example
  • Clean up the two examples of string fields that contain JSON data
  • Clean up the Bringing your own envelope examples

User experience

Please share what the user experience looks like before and after this change

modifcations_for_new_parser

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Sep 27, 2024
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 27, 2024
Copy link

boring-cyborg bot commented Sep 27, 2024

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@anafalcao anafalcao changed the title docs(parser): change parser documentation - WIP docs(parser): change parser documentation Sep 27, 2024
@anafalcao anafalcao changed the title docs(parser): change parser documentation docs(parser): change parser documentation - WIP Sep 27, 2024
@anafalcao anafalcao marked this pull request as ready for review September 27, 2024 17:56
@anafalcao anafalcao requested a review from a team as a code owner September 27, 2024 17:56
@leandrodamascena leandrodamascena marked this pull request as draft October 7, 2024 10:41
@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 17, 2024
Copy link
Contributor

⚠️Large PR detected⚠️

Please consider breaking into smaller PRs to avoid significant review delays. Ignore if this PR has naturally grown to this size after reviews.

@anafalcao anafalcao marked this pull request as ready for review October 21, 2024 12:12
@anafalcao
Copy link
Collaborator Author

Hey @leandrodamascena !
Can you please review this PR? Something is missing for this issue #5390, and I may need some direction to fix it.
Thanks

@leandrodamascena leandrodamascena changed the title docs(parser): change parser documentation - WIP docs(parser): change parser documentation Oct 21, 2024
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Hey @anafalcao! This is amazing to start reviewing this PR, indeed a great improvement in our documentation.

I see we have room to improve some areas and I left some comments. Can you please review and address them before we move to the next round of this review?

docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@anafalcao anafalcao left a comment

Choose a reason for hiding this comment

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

Thank you so much for the reviews @leandrodamascena ! Fixed your comments

docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Thanks for addressing to all the feedback, @anafalcao! I have some new comments in this second round!

Lets go!! We're almost ready to merge this PR.

docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
@boring-cyborg boring-cyborg bot added the github-actions Pull requests that update Github_actions code label Nov 7, 2024
@leandrodamascena leandrodamascena linked an issue Nov 7, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Nov 7, 2024

⚠️Large PR detected⚠️

Please consider breaking into smaller PRs to avoid significant review delays. Ignore if this PR has naturally grown to this size after reviews.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.16%. Comparing base (4a4bf17) to head (4d10ff8).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5262   +/-   ##
========================================
  Coverage    96.16%   96.16%           
========================================
  Files          229      229           
  Lines        10810    10810           
  Branches      2007     2007           
========================================
  Hits         10395    10395           
  Misses         327      327           
  Partials        88       88           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Hi @anafalcao! Congratulations on getting your first PR approved as a Powertools maintainer, this is a huge achievement in such a short time. I made a few small changes before approving it and let me tell you:

1 - We try to use active voice as much as possible, so in areas where you were saying "parser allows you to use XYZ", I changed it to "you can use XYZ in the parser utility".

2 - Some examples were importing things from Powertools instead of Pydantic, as it is better to import BaseModel directly from Pydantic than from Powertools, for example. Both work, but the customer can have more control over the imports.

3 - Mypy was complaining about some types, but don't worry, mypy is a nightmare sometimes.

4 - Some highlights were wrong and I fixed them.

Congratss!! 🚀 ⭐

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Left a few comments.

Thanks a lot for the work on this PR!

docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Nov 7, 2024

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Hi @dreamorosi! Thanks for such a detailed review, I pushed a new commit with the last few things before we merge.

docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
docs/utilities/parser.md Show resolved Hide resolved
docs/utilities/parser.md Outdated Show resolved Hide resolved
@dreamorosi dreamorosi self-requested a review November 7, 2024 17:05
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Great work @anafalcao, looking forward to see this merged.

We should probably bring these changes to the TS docs as well.

@leandrodamascena leandrodamascena merged commit e8d4859 into aws-powertools:develop Nov 7, 2024
12 checks passed
Copy link

boring-cyborg bot commented Nov 7, 2024

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation github-actions Pull requests that update Github_actions code size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Parser docs code snippets
3 participants