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

Remove some unused functions in dump.py #6365

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

jimman2003
Copy link
Contributor

refactor

Technical

Testing

Make a successful dump :)

Screenshot

Stakeholders

@cdrini

@jimman2003 jimman2003 marked this pull request as ready for review April 2, 2022 18:57
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Nice! Please add some unit tests for this function before making changes to it :) This code is really slow to run and non-trivial to test for staff. You can test it locally following the instructions at the top of oldump.sh.

@cdrini cdrini self-assigned this Apr 4, 2022
@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Apr 4, 2022
@jimman2003 jimman2003 requested a review from cdrini April 5, 2022 20:55
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Lgtm ; ran oldump.sh locally and compare before/after of authors and they were the same

@cdrini cdrini merged commit 561511b into internetarchive:master Apr 5, 2022
@jimman2003 jimman2003 deleted the ol_dumps_cleanup branch April 5, 2022 21:41
cclauss pushed a commit that referenced this pull request Apr 17, 2022
* Remove pgencode, unused

* Invert dict, remove now unused inverting function

* Remove flatten_dict,unused
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants