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

Refactoring upstream code to DRY it out a bit #207

Closed
wants to merge 18 commits into from

Conversation

webbnh
Copy link
Collaborator

@webbnh webbnh commented Sep 27, 2024

This PR is a series of refactorings and other modest changes to the upstream_*.py modules. The intention is that there be no overall functional change; however, internally, the changes remove a bunch of code duplication, shedding a little over 25% of the previous code.

Each of the first several commits is targeted at removing a particular code duplication. The commits after that are more code-quality/stylistic changes.

I recommend reviewing it one commit at a time.

A couple of notes on the tests:

  • The unit tests, via the mocking, have slightly more insight into the internal structure of the code than I might have wished, so I was not able to use them to validate my changes without also making some changes to them (which kind of undermines their purpose...); however, the test changes were fairly limited (the name of the function being mocked, the change to pass its second parameter by keyword-argument, and the module-path to where the requests package is to be patched), so I think the unit tests still did their job.
  • At this point, this PR is not expected to be merged as is -- it is mostly intended to record this work for future posterity -- so, there are no new tests for the new subroutines. Before merging this PR, new tests should be added.

@webbnh webbnh force-pushed the refactorings branch 2 times, most recently from 4272b5b to 44d7d8d Compare September 27, 2024 21:21
@ralphbean
Copy link
Member

/ok-to-test

@Zyzyx
Copy link
Collaborator

Zyzyx commented Oct 2, 2024

I submitted 2 PRs that reclaimed value for the last few commits (the styling). We should get the first 5 too, but they don't include the story point changes, so that's been tying me up.

@webbnh
Copy link
Collaborator Author

webbnh commented Oct 4, 2024

@Zyzyx, I've rebased this branch on the branch in #219 which is on the end of main, and I've confirmed that the unit tests run successfully on all the remaining commits. Thus, the commits from #219 also show up here, but the commits that you've cherry-picked from here and put on main are gone from here, as are the changes made for #208.

I think I've properly incorporated the Story Point work from #203, but I need to check my work.

@webbnh
Copy link
Collaborator Author

webbnh commented Oct 4, 2024

OK, it looks good.

@webbnh webbnh force-pushed the refactorings branch 2 times, most recently from d025ca5 to e4afd10 Compare October 7, 2024 20:56
@webbnh webbnh force-pushed the refactorings branch 2 times, most recently from a423360 to ec7892d Compare October 23, 2024 20:29
@webbnh webbnh force-pushed the refactorings branch 3 times, most recently from abb2ff4 to a7dcd3b Compare November 22, 2024 21:26
@webbnh
Copy link
Collaborator Author

webbnh commented Dec 9, 2024

This branch was merged as a series of individual PRs.

@webbnh webbnh closed this Dec 9, 2024
@webbnh webbnh deleted the refactorings branch December 9, 2024 19:23
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.

3 participants