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

Refactor mwe module #38

Merged
merged 18 commits into from
May 27, 2023
Merged

Refactor mwe module #38

merged 18 commits into from
May 27, 2023

Conversation

leobeeson
Copy link
Collaborator

Refactor MWE Module

Resolves #35 .

Situation

  • Some methods in MWE class produce IO side effects, which could potentially:
    1. Affect client applications' performance.
    2. Generate storage volume integration problems in cloud environments (e.g. running as lambda functions).

Solution

  1. Write unit tests ahead of refactoring to prevent breaking changes (i.e. refactor with TDD).
  2. Split methods that do more than one thing (at least unrelated things), such as a calculation alongside a read/write I/O operations.
  3. Rename variable and method names to generalise MWE extraction beyond just noun compounds.

NB

  • Setting this PR initially as draft request to push small changes related to the same issue.

Copy link
Owner

@meghdadFar meghdadFar left a comment

Choose a reason for hiding this comment

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

Thanks, @leobeeson! <3
See my comments below.
We also can drop __init__.py in the test directory. Pytest modules don't need it and Pytest docs recommend not including them. See here.

.vscode/launch.json Outdated Show resolved Hide resolved
tests/mwe/test_mwe.py Outdated Show resolved Hide resolved
tests/mwe/test_mwe.py Outdated Show resolved Hide resolved
wordview/mwes/mwe.py Outdated Show resolved Hide resolved
@leobeeson leobeeson marked this pull request as ready for review May 2, 2023 20:33
@leobeeson leobeeson requested a review from meghdadFar May 2, 2023 20:33
wordview/mwes/mwe.py Outdated Show resolved Hide resolved
wordview/mwes/mwe.py Outdated Show resolved Hide resolved
wordview/mwes/mwe.py Outdated Show resolved Hide resolved
Copy link
Owner

@meghdadFar meghdadFar left a comment

Choose a reason for hiding this comment

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

Please see my comments below.

@leobeeson leobeeson requested a review from meghdadFar May 18, 2023 17:58
@meghdadFar
Copy link
Owner

meghdadFar commented May 20, 2023

@leobeeson when running pre-commit, I get a bunch of PEP and static type errors thrown by pre-commit (flake8 and mypy). I wonder if you use the instructions here. I always use pre-commit and it's pretty helpful. First, isort and black edit the code to a nice format, and then mypy runs and checks static types and a bunch of other things, preventing many future run time errors, practically making the code much more stable.

If you install pre-commit following the instructions above, it runs each time that you commit code, blocking the commit until the issues are addressed.

For the already committed code, you can simply run pre-commit run --files <your_file.py>.

Here is a screenshot of the pre-commit (mypy, flake8) errors in am.py

Screenshot 2023-05-20 at 10 33 33

Copy link
Owner

@meghdadFar meghdadFar left a comment

Choose a reason for hiding this comment

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

Looks great, Thanks!

@meghdadFar meghdadFar merged commit f5bf642 into main May 27, 2023
0 of 3 checks passed
@meghdadFar meghdadFar deleted the refactor-mwe-module branch May 27, 2023 20:14
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.

Refactor MWE module.
2 participants