Skip to content

chore: ease maintenance of upcoming parser #118 #189

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

Merged
merged 22 commits into from
Oct 14, 2020

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Oct 4, 2020

Issue #, if available: #118

Description of changes:

Minor internal changes to ensure consistency across utilities, and ease maintenance before we write docs.

Checklist

  • Update docs
  • Update tests
  • Meet tenets criteria
  • PR title follows conventional commit semantics
  • Remove validators. We need an universal schema owned by Lambda for each event source, or else we'll likely break some customers if a few services change, as we won't be notified.
  • Snake_case. IIRC, EventBridge detailtype was the only odd one out but I'll double check if there are others
  • Kwargs over args. Some internal code is using args over kwargs - Moving to kwargs make it easier to refactor, and order of args wouldn't be impacted.
  • Docstrings. Enhance Example section in docstrings, and areas where we need more clarity for future maintenance
  • Raise own Validation Exception. For customers wanting to catch that exception (e.g. custom middlewares) they'll have to find Pydantic exception to catch it properly
  • Enhance imports. Envelopes and a few others can be turned into high level imports to clean up customers' code, plus DX.
  • Simplify internals of envelope: Parsing dict event, obj event, and string event share similar logic; wrap them under a single _parse
  • Fix PyCharm inspect issues
  • Test coverage. Validation errors aren't being tested as of now.
  • Refactor seemingly duplicates to ease maintenance
  • Chain exceptions with own exception to ease debugging
  • [Optional] Enhancing envelope selection. Time allowing, I'll experiment with providing a similar experience we have in the validator envelope utility envelope.<event_source>.
  • Revisit chain of try/catch: Allow exceptions to propagate, and use exception chaining to provide more details instead.
  • Expose Pydantic import system: Common imports from Pydantic such as validator, root_validator, and BaseModel should be part of the high level import system. Uncommon imports such as json, dataclasses, should be available nevertheless but ring fenced.
  • [Optional] Create a standalone parser. Time allowing, I'll experiment creating a standalone parser function with the same functionality the decorator
  • Remove flake8 polyfill. Double check whether a fresh Python env hits the issue you hit with that, since we don't use it
  • [Optional] Support for API Gateway, Kinesis, S3, and SNS. Likely to be after 1.7.0, but time allowing we can do that sooner.
  • Revisit Schemas naming. DynamoDBScheme vs DynamoDBSchema - might be best looked at when writing docs

Note: Decided to refactor test event data as a separate PR to prevent cluttering non-parser changes here. Data_classes and validator utility are partially using the same events, so we'll refactor tests to provide events to all tests as fixtures.

Breaking change checklist

RFC issue #:

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

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

@heitorlessa heitorlessa marked this pull request as draft October 4, 2020 16:45
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2020

Codecov Report

Merging #189 into develop will increase coverage by 0.39%.
The diff coverage is 95.06%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #189      +/-   ##
===========================================
+ Coverage    99.20%   99.59%   +0.39%     
===========================================
  Files           64       64              
  Lines         2505     2468      -37     
  Branches       113      108       -5     
===========================================
- Hits          2485     2458      -27     
+ Misses          14        8       -6     
+ Partials         6        2       -4     
Impacted Files Coverage Δ
...da_powertools/utilities/parser/schemas/__init__.py 100.00% <ø> (ø)
...mbda_powertools/utilities/parser/envelopes/base.py 86.48% <84.61%> (ø)
...ertools/utilities/parser/envelopes/event_bridge.py 85.71% <85.71%> (ø)
aws_lambda_powertools/utilities/parser/__init__.py 100.00% <100.00%> (ø)
..._powertools/utilities/parser/envelopes/__init__.py 100.00% <100.00%> (ø)
..._powertools/utilities/parser/envelopes/dynamodb.py 100.00% <100.00%> (ø)
...ambda_powertools/utilities/parser/envelopes/sqs.py 100.00% <100.00%> (ø)
...s_lambda_powertools/utilities/parser/exceptions.py 100.00% <100.00%> (ø)
aws_lambda_powertools/utilities/parser/parser.py 100.00% <100.00%> (ø)
...da_powertools/utilities/parser/schemas/dynamodb.py 100.00% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 585f708...64f3261. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #189 into develop will increase coverage by 0.67%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #189      +/-   ##
===========================================
+ Coverage    99.20%   99.87%   +0.67%     
===========================================
  Files           64       65       +1     
  Lines         2505     2451      -54     
  Branches       113      108       -5     
===========================================
- Hits          2485     2448      -37     
+ Misses          14        3      -11     
+ Partials         6        0       -6     
Impacted Files Coverage Δ
aws_lambda_powertools/utilities/parser/__init__.py 100.00% <100.00%> (ø)
..._powertools/utilities/parser/envelopes/__init__.py 100.00% <100.00%> (ø)
...mbda_powertools/utilities/parser/envelopes/base.py 100.00% <100.00%> (ø)
..._powertools/utilities/parser/envelopes/dynamodb.py 100.00% <100.00%> (ø)
...ertools/utilities/parser/envelopes/event_bridge.py 100.00% <100.00%> (ø)
...ambda_powertools/utilities/parser/envelopes/sqs.py 100.00% <100.00%> (ø)
...s_lambda_powertools/utilities/parser/exceptions.py 100.00% <100.00%> (ø)
...bda_powertools/utilities/parser/models/__init__.py 100.00% <100.00%> (ø)
...bda_powertools/utilities/parser/models/dynamodb.py 100.00% <100.00%> (ø)
...powertools/utilities/parser/models/event_bridge.py 100.00% <100.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 585f708...7480985. Read the comment docs.

@heitorlessa heitorlessa changed the title fix: comment out validators #118 improv: ease maintenance of parser #118 Oct 10, 2020
@heitorlessa heitorlessa changed the title improv: ease maintenance of parser #118 improv: ease maintenance of upcoming parser #118 Oct 10, 2020
@heitorlessa heitorlessa changed the title improv: ease maintenance of upcoming parser #118 chore: ease maintenance of upcoming parser #118 Oct 11, 2020
@heitorlessa heitorlessa marked this pull request as ready for review October 12, 2020 11:49
@heitorlessa
Copy link
Contributor Author

@cakepietoast ready when you are -- this is mostly semantics to ease maintenance after publishing the parser utility -- Could you take a look and see if the docstrings, import system, etc. are to what you'd expect too?

The additional conftest for parser, test data from data_classes, etc. will be dealt with in a separate PR - We need to consolidate all those JSON into fixtures available to all functional tests to prevent unnecessary I/O + easier to use.

@heitorlessa heitorlessa requested a review from to-mc October 12, 2020 11:52
@heitorlessa
Copy link
Contributor Author

Next steps before 1.7.0:

  • Review by @cakepietoast
  • Parser docs

Copy link
Contributor

@to-mc to-mc left a comment

Choose a reason for hiding this comment

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

LGTM - with the sole suggestion of renaming the parser function!

@heitorlessa heitorlessa merged commit 29ca18e into aws-powertools:develop Oct 14, 2020
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.

5 participants