Skip to content

Comments

Code quality improvements on sagemaker operators/hook#27453

Merged
eladkal merged 4 commits intoapache:mainfrom
aws-mwaa:vandonr/cleaning
Nov 4, 2022
Merged

Code quality improvements on sagemaker operators/hook#27453
eladkal merged 4 commits intoapache:mainfrom
aws-mwaa:vandonr/cleaning

Conversation

@vandonr-amz
Copy link
Contributor

No functional change here. I'm working on some new features in those files and wanted to extract the cosmetic changes in a separate PR to ease reviewing.

  • removed an unnecessary parameter init (self.config = config is done in the base class ctor)
  • removed some unnecessary mocks in tests
  • tried to factor some code in tests

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Nov 1, 2022
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unittest.TestCase is incompatible with @pytest.mark.parametrize which I'm using below.
Given the fact that this inheritance was unnecessary, I simply removed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Replace unittest classes by native class always welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mock is necessary but default behavior doesn't need to be changed.

Comment on lines +53 to 54
Copy link
Contributor Author

Choose a reason for hiding this comment

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

merging 2 tests in one

@vandonr-amz vandonr-amz marked this pull request as ready for review November 1, 2022 22:22
@vandonr-amz vandonr-amz requested a review from eladkal as a code owner November 1, 2022 22:23
@eladkal eladkal requested a review from o-nikolas November 3, 2022 06:51
@eladkal eladkal merged commit 531f2d2 into apache:main Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants