Skip to content

Conversation

@wannaphong
Copy link
Member

@wannaphong wannaphong commented Jul 18, 2020

I add provinces name of Thailand. (Thai and romanize)

(will resolve #467)

@pep8speaks
Copy link

pep8speaks commented Jul 18, 2020

Hello @wannaphong! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 72:80: E501 line too long (101 > 79 characters)
Line 74:80: E501 line too long (98 > 79 characters)
Line 93:1: W293 blank line contains whitespace

Comment last updated at 2020-07-23 03:04:52 UTC

@coveralls
Copy link

coveralls commented Jul 18, 2020

Coverage Status

Coverage increased (+0.03%) to 95.038% when pulling 362eb2d on Add-provinces into d27ff7d on dev.


return _THAI_THAILAND_PROVINCES

def provinces_all() -> list:
Copy link
Contributor

@p16i p16i Jul 19, 2020

Choose a reason for hiding this comment

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

maybe we can combine this function with provinces()?
To me, because these two functions are quite similar, we can have an argument letting the user to choose which list he/she wants.

For example, the signatures could be: provinces() and provinces(details=True).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

Copy link
Member

Choose a reason for hiding this comment

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

It's tricky here.

provinces() was designed to populate a set of province names in the first run (when _THAI_THAILAND_PROVINCES is still empty, it will read the names from a file with a name _THAI_THAILAND_PROVINCES_FILENAME) and for any other subsequent call it will just return _THAI_THAILAND_PROVINCES - to save processing time and avoid IO operation).

The modification breaks this.

May be we can modify the function in this way:

  • Has only one file (the CSV one) of Thai names and romanized names (province Ministry of Interior code, province abbreviation, province postal code, etc.)
  • In the first call, populate as set (or sorted list?) of name pairs to _THAI_THAILAND_PROVINCES.
  • In any other subsequent call, just use the existing _THAI_THAILAND_PROVINCES.
  • The return type/format will be determined by value in details=

Copy link
Member Author

Choose a reason for hiding this comment

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

@bact OK. I updated code.

@wannaphong wannaphong added this to the 2.3 milestone Jul 20, 2020
@p16i
Copy link
Contributor

p16i commented Jul 20, 2020

This is LGTM. I haven't checked the documentation yet. Should we merge it?

@wannaphong wannaphong requested a review from bact August 3, 2020 14:08
@wannaphong wannaphong merged commit 313aa92 into dev Aug 12, 2020
@p16i p16i deleted the Add-provinces branch August 12, 2020 07:14
@bact bact restored the Add-provinces branch August 13, 2020 19:10
@bact
Copy link
Member

bact commented Aug 13, 2020

Sorry for being so late, I was quite sick in the past weeks.

I made some code cleaning, fix the types, and remove the .txt file as the information there is duplicated with ones in .csv.

One question here please, what's the meaning of these field names in dict?

  • provinces_th
  • abridgement
  • provinces_en
  • HS

I propose:

  • name_th
  • abbr_th (abbr for abbreviation)
  • name_en
  • abbr_en

will this make sense?

Please continue discussion in #466

@bact bact added the enhancement enhance functionalities label Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement enhance functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

corpus.common.provinces() with details option

5 participants