Skip to content

Conversation

@fx-kirin
Copy link
Contributor

@fx-kirin fx-kirin commented Jan 8, 2020

I just would like to show you an example of the hash implementation.

#9 (comment)

@fx-kirin fx-kirin changed the title Hash every args Hash every args including pandas and numpy object. Jan 8, 2020
@fx-kirin fx-kirin changed the title Hash every args including pandas and numpy object. Hash every args including pandas and numpy objects. Jan 8, 2020
@shaypal5
Copy link
Member

This looks really nice, @fx-kirin !
Once you solve the Python 2.7 test problem (seems like a numpy-pandas version mismatch issue), and as long as you test it thoroughly - I've read the existing two tests, but do try to come up with more - we can merge this! :)

Of course, given that code coverage and quality do not go down. :)

@fx-kirin
Copy link
Contributor Author

I'm not sure how to fix that dependency problem because I didn't specify any version of them. I think that's the problem of pandas and numpy themselves. The modification of code looks quite ok to me. How do you think?

@shaypal5
Copy link
Member

You know what? Screw it. I'm dropping support for Python 2.7. It ended it life cycle and the community should move away from it anyway.

@shaypal5 shaypal5 merged commit 4d228b5 into python-cachier:master Jan 31, 2020
@shaypal5
Copy link
Member

Merged! I think it will pass all the tests and is A-OK test coverage and code quality wise!

Thank you for the contribution!

@shaypal5
Copy link
Member

Hey @fx-kirin ,

Unfortunately I had to roll back all hash-related additions due to issues #18 and #19 .
Any future attempt to implement this kind of feature should explicitly treat those issues, including testing for them using the code supplied by the reporter of the issues.

The hash branch contains all the changes before the revert commit.

Bummer. :/
But maybe this can be solved! :)

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.

2 participants