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

chore: general simplifications and cleanup #255

Merged
merged 8 commits into from
Jan 12, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Jan 4, 2021

Issue #, if available:

Description of changes:

  • Use simplified logic checks
  • Use list and dict comprehension
  • Remove useless assignments and return value immediately

Checklist

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

* Use simplified logic checks
* Use list and dict comprehension
* Return value directly
@michaelbrewer
Copy link
Contributor Author

Nothing functional just code style @heitorlessa

@codecov-io
Copy link

codecov-io commented Jan 4, 2021

Codecov Report

Merging #255 (3132bce) into develop (893068f) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #255      +/-   ##
===========================================
- Coverage    99.81%   99.81%   -0.01%     
===========================================
  Files           76       76              
  Lines         2772     2750      -22     
  Branches       112      112              
===========================================
- Hits          2767     2745      -22     
  Misses           4        4              
  Partials         1        1              
Impacted Files Coverage Δ
aws_lambda_powertools/logging/filters.py 100.00% <100.00%> (ø)
aws_lambda_powertools/logging/formatter.py 100.00% <100.00%> (ø)
aws_lambda_powertools/logging/logger.py 100.00% <100.00%> (ø)
aws_lambda_powertools/metrics/base.py 100.00% <100.00%> (ø)
aws_lambda_powertools/tracing/tracer.py 100.00% <100.00%> (ø)
...lambda_powertools/utilities/parameters/dynamodb.py 100.00% <100.00%> (ø)
...owertools/utilities/parser/envelopes/cloudwatch.py 100.00% <100.00%> (ø)
..._powertools/utilities/parser/envelopes/dynamodb.py 100.00% <100.00%> (ø)
...a_powertools/utilities/parser/envelopes/kinesis.py 100.00% <100.00%> (ø)
...ambda_powertools/utilities/parser/envelopes/sns.py 100.00% <100.00%> (ø)
... and 1 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 893068f...3132bce. Read the comment docs.

Copy link

@Nr18 Nr18 left a comment

Choose a reason for hiding this comment

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

Nice 👌 , @michaelbrewer just a question for my understanding did you replaced the variable assignment by using the return for another reason other than a style change?

I have read that variable assignment can be memory expensive so just wondering if reducing the memory footprint was a motive of this change as well?

aws_lambda_powertools/logging/logger.py Show resolved Hide resolved
aws_lambda_powertools/logging/logger.py Outdated Show resolved Hide resolved
@michaelbrewer
Copy link
Contributor Author

Nice 👌 , @michaelbrewer just a question for my understanding did you replaced the variable assignment by using the return for another reason other than a style change?

I have read that variable assignment can be memory expensive so just wondering if reducing the memory footprint was a motive of this change as well?

It also reduces the number of operations, maybe it is from my old java days when the compiler was not as smart. But to me it seems like an useless assignment that you just return immediately.

@michaelbrewer
Copy link
Contributor Author

@Nr18 does this look good otherwise. Maybe @heitorlessa can have a check as well

Copy link

@Nr18 Nr18 left a comment

Choose a reason for hiding this comment

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

LGTM

@heitorlessa
Copy link
Contributor

Hey @michaelbrewer thanks a lot! I'll go over that tomorrow - At a quick glance, some are great while others can make debugging difficult (e.g. logger X-Ray trace ID).

@heitorlessa heitorlessa added the internal Maintenance changes label Jan 12, 2021
@heitorlessa
Copy link
Contributor

heitorlessa commented Jan 12, 2021

Misread the tracer getenv line diff so we're good. I left others so we can actually fix the readability in finding a better formatter (Black) configuration -- here's an example where I personally think we lost readability to an extent, as the gain is minimal (perf, mem):

Before

        for record in parsed_envelope.Records:
            output.append(self._parse(data=record.Sns.Message, model=model))
        return output

After

return [self._parse(data=record.Sns.Message, model=model) for record in parsed_envelope.Records]

I'll create an issue to ensure these get adjusted automatically next time.

Thanks again @michaelbrewer and @Nr18, much appreciated!

@heitorlessa heitorlessa changed the title chore: General simplifications and cleanup chore: general simplifications and cleanup Jan 12, 2021
@heitorlessa heitorlessa merged commit 157a7bc into aws-powertools:develop Jan 12, 2021
heitorlessa referenced this pull request in heitorlessa/aws-lambda-powertools-python Jan 12, 2021
* develop:
  chore: general simplifications and cleanup (#255)
heitorlessa referenced this pull request in heitorlessa/aws-lambda-powertools-python Jan 17, 2021
* develop:
  chore: move env names to constant file (#264)
  docs: fix import (#267)
  feat: Add AppConfig parameter provider (#236)
  chore: update stale bot
  improv: override Tracer auto-capture response/exception via env vars (#259)
  docs: add info about extras layer (#260)
  feat: support extra parameter in Logger messages (#257)
  chore: general simplifications and cleanup (#255)
@michaelbrewer michaelbrewer deleted the feat-simplications branch February 22, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Maintenance changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants