8000 [feaLib] Make `include()` work like makeotf by miguelsousa · Pull Request #838 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[feaLib] Make include() work like makeotf #838

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
Merged
merged 1 commit into from
Feb 13, 2017
Merged

[feaLib] Make include() work like makeotf #838

merged 1 commit into from
Feb 13, 2017

Conversation

miguelsousa
Copy link
Collaborator

The way makeotf handles include() is to literally replace the statement with the contents of the file that is being referenced, therefore the paths need to be relative to the first file instead of relative to each other. I've adjusted the test files to cover other file location examples. The test results remain the same.

@anthrotype
Copy link
Member
anthrotype commented Feb 12, 2017 •
edited
Loading

Hi Miguel, thanks for your pull request.
I understand this is how makeotf, i.e. the reference implementation, works, but I'm 100 % convinced this is right.

As you know, the OpenType Feature File specification simply says:

http://www.adobe.com/devnet/opentype/afdko/topic_feature_file_syntax.html#3

If the file name were relative, then it would be appended to the directory of the including feature file.

The spec however does not say that "the paths need to be relative to the first file instead of relative to each other".

I wonder whether the spec should be modified to clarify this?

I personally think the way feaLib implements the include statement makes more sense, as it's similar to how the C preprocessor handles relative paths like #include "../myheader.h", ie. relative to the current source file.

/cc @brawer @readroberts

@anthrotype
Copy link
Member

I see you moved Tests/feaLib/data/include0.fea → Tests/feaLib/include0.fea.
We usually try to keep test data files inside the respective data subfolders.

@miguelsousa
Copy link
Collaborator Author

I don't oppose changing the current behavior, but it has been working like this for as long as I can remember, and we have a fair number of projects that rely on it. Other developers may depend on it too.

@miguelsousa
Copy link
Collaborator Author

@anthrotype yes, I did move include0.fea because I wanted to have an example where the included file is one level above the base file, and I don't see a way to do so without shuffling a few other files. So I opted for the simplest change. I understand that it puts the file outside of the data folder, but it seems like an edge case that we can live with.

@miguelsousa
Copy link
Collaborator Author

Here's a simple example to illustrate the situation that the PR is trying to address.
Consider the files arranged like this

└── DIR1
    ├── file1.fea
    ├── file2.fea
    └── DIR2
        └── base.fea

where base.fea includes file1.fea which in turn includes file2.fea.

For both makeotf and feaLib, the contents of base.fea are

include(../file1.fea);

and the contents of file2.fea are

@CLASS = [a b c];

But when it comes to the contents of file1.fea, makeotf requires

include(../file2.fea);

whereas feaLib needs it to be

include(file2.fea);

@brawer
Copy link
Collaborator
brawer commented Feb 13, 2017

Agree with @anthrotype that the makeotf behavior is funky, but I’d say supporting existing sources (and compatibility with makeotf) is more important. For the location of the test files, I’ll move them into the data directory in a follow-up change. Will also file a bug on the feature file spec, since it would be nice if the spec reflected reality.

@brawer brawer merged commit 75133fc into fonttools:master Feb 13, 2017
brawer added a commit that referenced this pull request Feb 13, 2017
@brawer
[feaLib] Move test file include0.fea into test data subdirectory
ff188b2 
Move the other include files into a new subdirectory inside `data`.
Mention the changed behavior of include statements in NEWS.
See #838.
This was referenced Feb 13, 2017
Merged
Closed
@brawer
Copy link
Collaborator
brawer commented Feb 13, 2017

It might be good to clarify the spec, since the path resolution is quite unusual compared to other languages. Filed adobe-type-tools/afdko#164.

brawer added a commit that referenced this pull request Feb 13, 2017
@brawer
[feaLib] Move test file include0.fea into test data subdirectory
7cf22d0 
Move the other include files into a new subdirectory inside `data`.
Mention the changed behavior of include statements in NEWS.
See #838.
@miguelsousa miguelsousa deleted the feaLib-include branch February 13, 2017 11:02
@miguelsousa
Copy link
Collaborator Author

Thanks Sasha.

@anthrotype
Copy link
Member

Thank you Miguel and Sascha.

zjusbo pushed a commit to zjusbo/fonttools that referenced this pull request Mar 8, 2017
@brawer @zjusbo
[feaLib] Move test file include0.fea into test data subdirectory
97e27ac 
Move the other include files into a new subdirectory inside `data`.
Mention the changed behavior of include statements in NEWS.
See fonttools#838.
@miguelsousa miguelsousa mentioned this pull request Jun 13, 2017
Closed
@anthrotype anthrotype mentioned this pull request Feb 22, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miguelsousa @anthrotype @brawer
0