Skip to content

Add filter for languages with ll_CC description #133

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

Merged
merged 3 commits into from
Aug 28, 2020

Conversation

columbarius
Copy link
Contributor

Implementing the proposed solution in #125

tldr.py Outdated
os.environ.get(
'LANG',
'C'
).split('.')[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does anyone know how to apply this filter with chain notation for a nicer syntax like
.split('.')[0].apply(filter_languages)?
Couldn't find a solution with a quick search.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move the .split('.')[0] step into filter_languages as its first line, given we're doing a similar step below, and it would be good to condense it to one place, and would allow simplifying the double map below into a singular one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense.

tldr.py Outdated
os.environ.get(
'LANG',
'C'
).split('.')[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move the .split('.')[0] step into filter_languages as its first line, given we're doing a similar step below, and it would be good to condense it to one place, and would allow simplifying the double map below into a singular one.

tldr.py Outdated
'C'
).split('_')[0]

def filter_languages(lang):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of this name as it feels like what you might think it does from the name (given a list of languages, filter it down) is not the same as what it actually does (given a language, rewrite it to appropriate form). get_language_code(language) or get_tldr_language_code(language) maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, for me a filter is something which modifies or removes an input and outputs the result, but I see your point.

@MasterOdin
Copy link
Collaborator

Thanks for this! Sorry for taking a bit to review, some minor comments, but definitely an improvement on the state of things!

@columbarius
Copy link
Contributor Author

No Problem. I noticed, open source can take it's time, since it's freetime for most of us. Thanks for the review.

@columbarius columbarius requested a review from MasterOdin August 17, 2020 17:20
Copy link
Collaborator

@MasterOdin MasterOdin left a comment

Choose a reason for hiding this comment

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

Thanks again for your contributions!

@MasterOdin MasterOdin merged commit ab3b318 into tldr-pages:master Aug 28, 2020
@columbarius
Copy link
Contributor Author

Thanks for merging!

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