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.

Sign up for GitHub

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

Jump to bottom

refactoring to use external gflanguages module #511

Merged
merged 1 commit into from
Mar 9, 2022
Merged

refactoring to use external gflanguages module #511

merged 1 commit into from
Mar 9, 2022
+31 −22,940

Conversation

felipesanches
Copy link
Member

@felipesanches felipesanches force-pushed the use_gflanguages_module branch 2 times, most recently from 0507b34 to b752a22 Compare March 3, 2022 04:16
@m4rc1e
Copy link
Collaborator

m4rc1e commented Mar 3, 2022

Thanks for this.

If we're all happy, we should transfer gflanguages to googlefonts

@m4rc1e
Copy link
Collaborator

m4rc1e commented Mar 3, 2022

It would also be nice to do this for the axis registry as well.

@felipesanches
Copy link
Member Author

Thanks for this.

If we're all happy, we should transfer gflanguages to googlefonts

Yes! Be sure to take a look at #510 where I provide a longer explanation of the rationale behind this pull request. The transfer of gflanguages to googlefonts is also mentioned there.

@felipesanches
Copy link
Member Author

It would also be nice to do this for the axis registry as well.

I agree. And it is worth mentioning here (for the public record) that the same approach was already taken for the glyphset files a while ago: https://github.com/googlefonts/glyphsets/

@felipesanches felipesanches mentioned this pull request Mar 3, 2022
Closed
@m4rc1e m4rc1e self-requested a review March 4, 2022 11:28
@m4rc1e
Copy link
Collaborator

m4rc1e commented Mar 4, 2022

I get the following traceback when trying to use gftools add-font:

(venv) Marcs-Air-2:tools marcfoley$ gftools add-font ~/Type/fonts/ofl/abeezee/
Traceback (most recent call last):
  File "/Users/marcfoley/Type/tools/bin/gftools-add-font.py", line 286, in <module>
    app.run(main)
  File "/Users/marcfoley/Type/tools/venv/lib/python3.9/site-packages/absl/app.py", line 312, in run
    _run_main(main, args)
  File "/Users/marcfoley/Type/tools/venv/lib/python3.9/site-packages/absl/app.py", line 258, in _run_main
    sys.exit(main(argv))
  File "/Users/marcfoley/Type/tools/bin/gftools-add-font.py", line 273, in main
    lang_support.LoadLanguages(os.path.join(FLAGS.lang, 'languages'))
  File "/opt/homebrew/Cellar/python@3.9/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python3.9/posixpath.py", line 76, in join
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not NoneType

felipesanches added a commit to felipesanches/gflanguages that referenced this pull request Mar 4, 2022
@felipesanches
Simplify/Update API
2585b5a 
  - Simplify API by removing the `lang_support` module. Now one does `from gflanguages import LoadLanguages` instead of `from gflanguages.lang_support import LoadLanguages` (issue googlefonts#6)
  - Also, all `Load_*` methods now accept base_dir as optional argument. (motivated by: googlefonts/gftools#511 (comment))
  - This will be the v0.3.0 API.
@felipesanches felipesanches mentioned this pull request Mar 4, 2022
Merged
felipesanches added a commit to felipesanches/gflanguages that referenced this pull request Mar 4, 2022
@felipesanches
Simplify/Update API
b9a6f3a 
  - Simplify API by removing the `lang_support` module. Now one does `from gflanguages import LoadLanguages` instead of `from gflanguages.lang_support import LoadLanguages` (issue googlefonts#6)
  - Also, all `Load_*` methods now accept base_dir as optional argument. (motivated by: googlefonts/gftools#511 (comment))
  - This will be the v0.3.0 API.
felipesanches added a commit to felipesanches/gflanguages that referenced this pull request Mar 4, 2022
@felipesanches
Simplify/Update API
ac137c8 
  - Simplify API by removing the `lang_support` module. Now one does `from gflanguages import LoadLanguages` instead of `from gflanguages.lang_support import LoadLanguages` (issue googlefonts#6)
  - Also, all `Load_*` methods now accept base_dir as optional argument. (motivated by: googlefonts/gftools#511 (comment))
  - This will be the v0.3.0 API.
felipesanches added a commit to googlefonts/lang that referenced this pull request Mar 4, 2022
@felipesanches
Simplify/Update API
ea009d0 
  - Simplify API by removing the `lang_support` module. Now one does `from gflanguages import LoadLanguages` instead of `from gflanguages.lang_support import LoadLanguages` (issue #6)
  - Also, all `Load_*` methods now accept base_dir as optional argument. (motivated by: googlefonts/gftools#511 (comment))
  - This will be the v0.3.0 API.
@felipesanches felipesanches force-pushed the use_gflanguages_module branch from b752a22 to b9d8ef1 Compare March 4, 2022 21:47
@felipesanches
Copy link
Member Author

@m4rc1e this is ready now for another code-review.

@m4rc1e
Copy link
Collaborator

m4rc1e commented Mar 7, 2022

I'm still getting the same traceback as above when running gftools add-font. Please inspect.

felipesanches added a commit to felipesanches/gflanguages that referenced this pull request Mar 9, 2022
@felipesanches
Use default DATA_DIR if user passes base_dir=None
d14e1d4 
To make it easier to use the API and to avoid problems like this:
googlefonts/gftools#511 (comment)
@felipesanches felipesanches mentioned this pull request Mar 9, 2022
Merged
felipesanches added a commit to googlefonts/lang that referenced this pull request Mar 9, 2022
@felipesanches
Use default DATA_DIR if user passes base_dir=None
7a223ee 
To make it easier to use the API and to avoid problems like this:
googlefonts/gftools#511 (comment)
@felipesanches felipesanches force-pushed the use_gflanguages_module branch from b9d8ef1 to 95fde71 Compare March 9, 2022 06:41
@felipesanches
Copy link
Member Author

I'm still getting the same traceback as above when running gftools add-font. Please inspect.

I've made the API more robust:
googlefonts/lang#8

Please pip install -r requirements.txt again so that you have the latest gflanguages==0.4.0. And then gftools add-font will work.

@felipesanches felipesanches changed the title refactoring to use external gflanguages module refactoring to use external gflanguages module Mar 9, 2022
m4rc1e
m4rc1e approved these changes Mar 9, 2022
View reviewed changes
Copy link
Collaborator

@m4rc1e m4rc1e left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@m4rc1e m4rc1e merged commit 4242d8d into main Mar 9, 2022
@m4rc1e m4rc1e deleted the use_gflanguages_module branch March 9, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m4rc1e m4rc1e m4rc1e approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@felipesanches @m4rc1e