Allow insertion markers for autogenerated features #352
Conversation
moyogo
force-pushed
the
features-insert-markers
branch
from
56ce482 to
b129f52
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@moyogo can't review this until tests pass |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
moyogo
force-pushed
the
features-insert-markers
branch
from
3edd970 to
3952f5f
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@moyogo I think googlebot wants you to say "I consent."
|
|
@googlebot I consent. |
|
LGTM I think 🤔 |
|
hold on the merge, i'd like to take a look (tomorrow hopefully) |
| for _, feature in sorted(features.items()): | ||
| feaFile.statements.append(feature) | ||
|
|
||
| insertion_tag2index = ufo2ft.featureWriters.findFeatureInsertionMarkers(feaFile) |
There was a problem hiding this comment.
this only supports "### INSERT ... ###" comments for the MarkFeatureWriter. What about KernFeatureWriter (or future feature writers that we haven't written yet)?
Shouldn't this parsing be done once before all automatic features are generated, instead of at the level of the singular feature writer?
Or maybe even better, instead of the current finding all insertion markers and building a mapping of tag:index, we could move this logic to a method of BaseFeatureWriter which gets called just before compilation from within BaseFeatureWriter.setContext method, and it will store the index of the insertion comment in the self.context namespace for that writer; so no need for a mapping, as the link between the statement index and the feature tag is implicit by the association with the current writer's context.
There was a problem hiding this comment.
actually, each feature writer can write multiple feature tags (e.g. "mark", "mkmk", or "kern" and "dist"), so we'd still need to store a mapping from tag to index in the writer's self.context. But each different writer should in theory only be interested in the insertion tags that pertains to it, not all of them. Unless... the placement of class definitions poses a problem (see the other comment https://github.com/googlefonts/ufo2ft/pull/352/files#r560061188)
|
|
||
| if insertion_tag2index: | ||
| # Write the class definitions at the first feature insertion point, | ||
| # no matter what it is. |
There was a problem hiding this comment.
hm I forgot why we need to write class definitions "at the first feature insertion point, no matter what it is"?
This makes the finding-insertion-index logic global as opposed to independent for each writer (I would prefer the latter if possible).
There was a problem hiding this comment.
I think because there's no guarantee they're written anywhere before that 🤔 I remember stumbling over this.
|
Replaced by #458 |
Tackle #351.