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

Change empty language string to None in tldr.py #122

Merged
merged 3 commits into from
Jun 17, 2020

Conversation

columbarius
Copy link
Contributor

Tldr.py uses the value None for a language, when no language is set. In line 160 languages is set to '' if no environment is set.

  languages = os.environ.get('LANGUAGES', '').split(':')

So the languages list looks like this:

['de', '']

which results in wrong language handling in line 107 of get_line_url() and a wrong url like

https://raw.githubusercontent.com/tldr-pages/tldr/master/pages./linux/unzip.md

. My suggested fix is to substitute '' to None.

tldr.py Outdated Show resolved Hide resolved
@zlatanvasovic
Copy link
Contributor

Good proposal. Can you make it work inside the current loop as @MasterOdin suggested?

@columbarius
Copy link
Contributor Author

columbarius commented Jun 7, 2020

I'm not sure if this is the desired intent. With the following fix, I'm just receiving pages in my native language (DEFAULT_LANG) and none from the common/english one as described as a fallback on the readme.

        filter(lambda x: not (x == 'C' or x == ''), languages)

As I read it, the default should always be the native language and the common/english repository, except TLDR_LANGUAGE is set, then only pages in this language should be used.

So probably the default value for os.environ.get in line 160

    languages = os.environ.get('LANGUAGES', '').split(':')

should be None instead of '', but that will conflict with the string manipulation afterwards. So my suggestion aims, instead of analyzing the return value of os.environ.get, to handle the unset case separately to do all string manipulation and afterwards fix the empty string to None, which I would expect for an unset LANGUAGES envvar.

Edit:
I looked a little bit further into the code and noticed that DEFAULT_LANG is defined by TLDR_LANGUAGE and LANG. And this gets added to to the elements derived from LANGUAGES. Shouldn't TLDR_LANGUAGE be the only language used if set?
As far, as i read it, the priority order is:

  1. TLDR_LANG (exclusive, if set)
  2. LANGUAGES with LANG in first place and None in last as fallback
  3. None if none of the above are set.

I will update my pull request, if you can confirm, that i got the priority correctly.

@MasterOdin MasterOdin merged commit 764114b into tldr-pages:master Jun 17, 2020
@MasterOdin
Copy link
Collaborator

Thanks for this. I do agree on having a default of English always and given the proposed changes on tldr-pages/tldr#4101 will be looking to do this in a follow-up work.

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.

None yet

3 participants