Fix conflict from #25 in v0.0.1-packaging #28
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Important changes: - A number of the type hints on function/method parameters were incorrect (producing errors detected statically by mypy, or by temporarily instrumenting the code at runtime with typeguard). This fixes most of those. - A few names of attributes and function parameters were renamed in one place but not another. The affected function parameters, which were mostly on local helper functions, were unintentionally unused, while their old now-unbound names were still referenced. This went undetected because it was on code paths that weren't often exercised. This fixes most (maybe all) cases of that. More minor changes (only made in code already being changed): - This adds a few more type hints in places where it is simple to do so and improves consistency, and in some places where type checkers like mypy consider it an error for them to be absent. - In some cases, the incorrect type annotations or incomplete renames appear to have arisen in part due to parameter names with non-obvious meanings, or that vary in meaning across code that otherwise appeared similar. In places where that looks like that may have been the case, this renames them accordingly.
This specifies the latest versions compatible with Python 3.7 of each of the packages listed in requirements.txt. It adds torchdata, which was needed (and not always automatically installed) at least on Python 3.7 (and does not break later versions). It removes tornado, which does not appear to be required.
Fix some incorrect type hints and incomplete renames
Update pinned dependencies
|
I'm going to approve and merge so we keep the changes consistent. Refactoring this into a package is a sizeable change but having the appropriate type hints will be helpful. Thanks for the PR! |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a request to merge commits into the
v0.0.1-packagingfeature branch, not intomain.I noticed that the changes I made in #25 caused
mainandv0.0.1-packagingto diverge, such that a nontrivial merge conflict would arise onv0.0.1-packaging. Merging this pull request into thev0.0.1-packagingbranch will resolve that conflict, preserving both changes. This pull request supplies a merge commit, d7633f8, in which the conflicts are resolved. I have tested this code.The purpose of this pull request is to prevent my changes in #25 from delaying #24 unnecessarily or causing extra work for @zbloss (who has been working on #24 in
v0.0.1-packaging). Therefore, my suggestion is that that this pull request should only be merged by, or after review from, @zbloss.@zbloss: I hope you find this useful, but I understand that pull requests against feature branches are somewhat unusual and not part of everyone's preferred workflow. (Likewise, it is possible that you have already resolved this conflict locally even as I am making his PR, etc.) Please feel free to close this pull request if you decide, for any reason, that you don't want to merge it into your branch.