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

Proposing changes to alex.utils & cleaning up. #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ticcky
Copy link
Contributor

@ticcky ticcky commented Mar 12, 2015

I went through alex.utils, deleted some obviously unused code and marked some changes that I would like to make to improve Alex's codebase. Please let me know what you think of them.

Please do NOT merge this pull request. It is only proposal for changes.

Thanks!

@landscape-bot
Copy link

Code Health
Repository health increased by 0.01% when pulling dbe2e14 on cleanup into 06a0695 on master.

@@ -28,7 +28,7 @@
import sys
import re
from add_cities_to_stops import load_list, get_city_for_stop
from alex.utils.various import remove_dups_stable
from alex.utils.various import list_remove_duplicates_keep_ordering
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep the old name – I think it describes the function sufficiently and the new one makes lines too long ;-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that I had to study the code of the method to understand what is it doing - it was not apparent to me on the first sight it is working on a list. So how about list_remove_dups_stable ?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I've just realized that I wrote this function... maybe that's why it seemed so clear to me 😄 . So I'm probably not the right one to judge... remove_list_dups_stable would sound slightly better to me, but i'm OK with both.

@oplatek
Copy link
Member

oplatek commented Mar 13, 2015

Proposed new change:
Replace our Counter class with stdlib collections.Counter
at file:
https://github.com/UFAL-DSG/alex/blob/cleanup/alex/utils/cache.py#L23

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.

4 participants