Skip to content

refactor: move simulator utils functions #772

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 1 commit into from
Nov 7, 2023

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Sep 13, 2023

Motivation

The events simulator CLI tool was using some functions defined in the tests module, which is not copied to the Docker image. To make the tool available via Docker, those functions must be moved to another module.

Acceptance Criteria

  • Move functions from tests module to utils/simulator module
  • Update imports
  • Add custom check to make sure tests are not imported in hathor
  • Small changes to fix linter issues with added typings

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco self-assigned this Sep 13, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (769d255) 85.16% compared to head (5a38b60) 85.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #772      +/-   ##
==========================================
+ Coverage   85.16%   85.22%   +0.06%     
==========================================
  Files         284      285       +1     
  Lines       22576    22655      +79     
  Branches     3430     3444      +14     
==========================================
+ Hits        19226    19308      +82     
  Misses       2667     2667              
+ Partials      683      680       -3     
Files Coverage Δ
hathor/simulator/tx_generator.py 96.05% <100.00%> (ø)
hathor/simulator/utils.py 98.73% <98.73%> (ø)

... and 4 files with indirect coverage changes

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

@glevco glevco marked this pull request as ready for review September 14, 2023 02:13
@glevco glevco force-pushed the chore/move-simulator-utils branch from eda0545 to e672226 Compare September 15, 2023 22:00
jansegre
jansegre previously approved these changes Sep 19, 2023
Copy link
Member

@jansegre jansegre left a comment

Choose a reason for hiding this comment

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

Looks good to me. Two minor points:

  • This should probably be named a refactor;
  • Should we add a linter check to prevent from tests* import or import tests*?

@glevco glevco force-pushed the chore/move-simulator-utils branch 2 times, most recently from f383165 to 2dbc6b3 Compare September 20, 2023 20:03
@glevco glevco changed the title chore: move simulator utils functions refactor: move simulator utils functions Sep 20, 2023
@glevco glevco force-pushed the chore/move-simulator-utils branch 2 times, most recently from 85c5ae3 to 7e2d1af Compare September 20, 2023 20:35
@glevco
Copy link
Contributor Author

glevco commented Sep 20, 2023

  • This should probably be named a refactor;
  • Should we add a linter check to prevent from tests* import or import tests*?

@jansegre both done!

@glevco glevco force-pushed the chore/move-simulator-utils branch from 7e2d1af to fc27813 Compare September 20, 2023 20:37
@glevco glevco requested review from jansegre and msbrogli September 20, 2023 20:40
@glevco glevco force-pushed the chore/move-simulator-utils branch from fc27813 to 6bd92a1 Compare September 20, 2023 20:56
jansegre
jansegre previously approved these changes Sep 22, 2023
msbrogli
msbrogli previously approved these changes Sep 22, 2023
@glevco glevco force-pushed the chore/move-simulator-utils branch 2 times, most recently from 050373e to 4c436e3 Compare September 24, 2023 16:30
@glevco glevco dismissed stale reviews from jansegre and msbrogli via c2e4593 September 27, 2023 15:40
@glevco glevco force-pushed the chore/move-simulator-utils branch from 4c436e3 to c2e4593 Compare September 27, 2023 15:40
jansegre
jansegre previously approved these changes Sep 27, 2023
@glevco glevco force-pushed the chore/move-simulator-utils branch from 1981890 to 1d4c04c Compare September 27, 2023 17:57
msbrogli
msbrogli previously approved these changes Sep 28, 2023
@glevco glevco force-pushed the chore/move-simulator-utils branch from 1d4c04c to f093a77 Compare September 29, 2023 03:33
@glevco glevco added the refactor label Oct 3, 2023
@glevco glevco marked this pull request as draft October 10, 2023 03:18
@glevco
Copy link
Contributor Author

glevco commented Oct 10, 2023

Converted to draft as we wait other PRs to be merged before this one.

@glevco glevco force-pushed the chore/move-simulator-utils branch from f093a77 to cacba12 Compare November 7, 2023 14:29
@glevco glevco force-pushed the chore/move-simulator-utils branch from cacba12 to 5a38b60 Compare November 7, 2023 14:31
@glevco glevco marked this pull request as ready for review November 7, 2023 15:43
@glevco glevco merged commit 8671304 into master Nov 7, 2023
@glevco glevco deleted the chore/move-simulator-utils branch November 7, 2023 21:28
@jansegre jansegre mentioned this pull request Nov 13, 2023
2 tasks
This was referenced Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants