version 2.0 #47
version 2.0 #47
Conversation
|
@psolin I'm happy to fork & take over full maintenance of this package if you no longer wish to spend any time on it? |
|
It’s been a day since you created this pull request. I’ll take a look and merge it. |
There was a problem hiding this comment.
Since this is a large refactoring, Readme needs to be updated if the old “silly” method is going away.
Is there a way to do this without calling get_terms() and storing it in a variable? 2.0 may need to be SQLite-driven given the amount of data needing to be referenced.
Iso test also removed
|
Take a look and let me know if this works. |
|
I don't see why SQLite or such would be needed for anything, it's a huge overkill. The terms are loaded & processed when application starts and are then kept in memory as regular Python objects. There's just I think three hundred short unique text terms, and then the country names - this is nothing. |
|
I agree. I ended up removing that and just edited the terms in the list. |
|
Regarding README, I did not update it yet since I did not add any deprecation warnings of the old way either. I was thinking of suggesting a deprecation in a future minor PR. If there's something you'd like changed in the PR, please add some review notes in the PR. I can then either make the changes or first explain why it's done this way. I guess that's the whole idea of collaborative PRs. As reviewer & package author, you can then choose to accept or reject the changes. There's no need for the reviewer to spend his time to actually fix anything in a PR - rather, that's the job of the person submitting the PR. Besides changing README & adding some nice termdata improvements, It seems to me you've also changed the imports in: cleanco/classify.py Is there a reason for this? Those changes break the package & tests. The setup.py file seems to be changed also. Is that intentional? |
|
You can revert those import lines. I was having issues running the package, but I think that I needed to do it in a virtualenv instead of how I was doing it. Everything did check out from my testing. I'm not sure what happened with setup.py, but that can be reverted as well. Check the readme edits that I made and let me know if that write up makes sense at all. |
|
Ok given your ok for the PR content, I just first applied your termdata changes to master and then committed my original 2.0 PR changes on top of it. So this PR can be just closed & forgotten. If we want to deprecate the old class-based approach, it's easy to do now whenever you want. |
|
The 2.0 branch can be deleted, unless we want to save your README changes from there first. |
cleancomodule.I also created a new branch from current master, so if we want to make a final release based on old codebase, or if someone wants to contribute to it still, that can be done in the
1.4branch.