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

Improve soft-dotted-i check: Add affected languages #4140

Closed
wants to merge 14 commits into from

Conversation

yanone
Copy link
Collaborator

@yanone yanone commented May 22, 2023

Description

This pull request addresses the problems described at issue #4085

PR is in draft because waiting for shaperglot dependency to appear on PyPI. (cc @simoncozens)

To Do

  • update CHANGELOG.md
  • wait for all checks to pass
  • request a review

@yanone yanone linked an issue May 25, 2023 that may be closed by this pull request
@yanone yanone marked this pull request as ready for review May 26, 2023 13:30
@yanone yanone requested a review from felipesanches May 26, 2023 13:30
@yanone
Copy link
Collaborator Author

yanone commented May 26, 2023

@felipesanches Checks complain about shaperglot, but I don't understand why. Could you please take a look? I can install it on my computer. It may be because the package on PyPI references a git-hosted package, which pip complains about if you install it with explicit version, but passes without explicit version. All very weird. Simon says he'll push an updated package soon.

The other complaint is from unrelated changes and are present in the current main codebase, please let us merge this anyway. I already made an issue about that and assigned Marc, who made those changes.

But generally, from my side, this is done.

@yanone
Copy link
Collaborator Author

yanone commented Jun 13, 2023

@felipesanches This is ready. Please pay extra attention to the changes in setup.py. There was a merge conflict and this is how I resolved it manually. Maybe you recognize the new setup. Let me know if I shall change anything.

Most tests are passing now, except Python 3.8 with an import error that I don't understand.

I loosened the protobuf dependency because there was a dependency resolution error earlier probably involving shaperglot. If this is a problem we need to look at the dependencies separately to see if we can tighten or loosen their protobuf.

Thank you.

@felipesanches
Copy link
Collaborator

I will only be able to review this later tonight, or tomorrow morning. In the mean time, it would be useful to rebase it all against the main branch, since we had a large commit touching practically all files of the codebase yesterday when we finally adopted the Black code formatter.

If you want to, you can rebase it yourself. Otherwise, I can do it once I'm back here. Just let me know what do you prefer. Also, from now on we'll be using black, so it would be good to run pip install black and black . --extend-exclude=*_pb2.py and commit the changes made by the auto-formatter.

@yanone
Copy link
Collaborator Author

yanone commented Jun 13, 2023

I’m scared. Okay, I will try. If that somehow causes too many conflicts, I’ll make a new branch and PR against the latest main. I’m not the biggest friends with branches and rebasing.

@yanone
Copy link
Collaborator Author

yanone commented Jun 13, 2023

Okay, I committed the blacked changes. Is the rebasing really necessary now when the code of the PR itself is good to go? The branch gets deleted afterwards.

I don't get this one.

@@ -17,7 +17,7 @@ opentype-sanitizer==9.1.0
opentypespec==1.9.1
packaging==23.1
pip-api==0.0.30
protobuf==3.20.3
protobuf>=3.19.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove this change

@@ -27,3 +27,4 @@ ufolint==1.2.0
unicodedata2==15.0.0
vharfbuzz==0.2.0
ufo2ft==2.32.0
shaperglot>=0.2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please always use == on requirements.txt.
And >= only on setup.py, when needed.

"gflanguages>=0.3.0", # there was an api simplification/update on v0.3.0 (see https://github.com/googlefonts/gflanguages/pull/7)
"glyphsets>=0.5.0",
"lxml",
"munkres", # for interpolation compatibility checking
Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR is bringing back all the contents of the setup.py from before a recent commit this week that implemented profile-specific extras. I can fix this one for you in a followup PR, if you are OK with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well... actually not a followup PR, but a new PR to replace this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. I will deal with this one today.

@felipesanches
Copy link
Collaborator

This continues at #4208

@khaledhosny khaledhosny deleted the import-soft-dotted-i-check branch October 9, 2023 13:30
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.

com.google.fonts/check/soft_dotted may be too strict for now for GF
2 participants