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

Add OpenTypeFeature suppport #864

Merged
merged 10 commits into from
Jul 17, 2019
Merged

Add OpenTypeFeature suppport #864

merged 10 commits into from
Jul 17, 2019

Conversation

thomgb
Copy link
Contributor

@thomgb thomgb commented Jan 24, 2018

We, type designers, use opentypefeature code. It would be nice to have support for this.

@thomgb
Copy link
Contributor Author

thomgb commented Jan 29, 2018

Hello again. It would be very nice if someone can click the button so our websites can benefit from the new lexer. Pretty please!

@typemytype
Copy link

what should be done to get OpenTypeFeature support?

thanks

@typemytype
Copy link

beep!

any thoughts?

the spec is maintained by Adobe see https://www.adobe.com/devnet/opentype/afdko/topic_feature_file_syntax.html

Copy link
Contributor

@vidarh vidarh left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your submission. I'm helping triage pull requests to see if we can work down the backlog, finally. I've got only a minor comment here with respect to removing some unused/commented out lines. Other than that it looks ready to me.

lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
@vidarh
Copy link
Contributor

vidarh commented Jan 13, 2019

Actually, sorry, one more little nitpick: Can you add "# frozen_string_literal: true" to the ruby files. This is down to how long the PR has open, sorry, it was added project wide a while back.

@typemytype
Copy link

hope these requested changes are oke...

thanks!!

@gferreira
Copy link

hi @vidarh, any updates on this? can this lexer finally be merged? thanks!

Copy link
Contributor

@pyrmont pyrmont left a comment

Choose a reason for hiding this comment

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

@thomgb I'm sorry it's taken so long to get back to you on this. I've left some specific comments below.

At a more general level:

  1. If you rebase this against the current master, you should pick up the changes that will allow Travis to run the tests properly.

  2. To avoid ambiguity warnings, could you denote regular expressions with %r? So rather than rule /.../, you'd write rule %r/.../.

Let me know if you have any questions!

lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/opentypefeature.rb Outdated Show resolved Hide resolved
@pyrmont
Copy link
Contributor

pyrmont commented Jul 11, 2019

@gferreira Sorry it's been taking so long. We've been working through the backlog but I'm afraid I hadn't got to this one yet. I've submitted feedback on the lexer and will wait to see how that goes! :)

@pyrmont pyrmont added the author-action The PR has been reviewed but action by the author is needed label Jul 11, 2019
@gferreira
Copy link

@pyrmont many thanks for your feedback, updates by @typemytype coming soon… cheers!

@typemytype
Copy link

Ive rebased to the current master and applied requested changes

thanks for the thorough comments!

@ashmaroli
Copy link
Contributor

ashmaroli commented Jul 11, 2019

@typemytype There are some linting issues. Refer the Travis build logs for details or run
bundle exec rubocop locally and use %r syntax for regular expressions passed as arguments.

@pyrmont
Copy link
Contributor

pyrmont commented Jul 12, 2019

@typemytype Regarding the point that @ashmaroli was raising, to avoid RuboCop errors, you need to write your regular expressions in such a way so that they are not potentially confused with division using the / character. Most lexers do this by putting %r before the initial /. So instead of /.../, you'd write %r/.../. Let me know if you have any questions!

@typemytype
Copy link

@ashmaroli thanks %r did it!

 359 files inspected, no offenses detected

@pyrmont pyrmont merged commit 0a736e5 into rouge-ruby:master Jul 17, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jul 17, 2019

@typemytype After looking at the Adobe specification more closely, I thought it was more correct to call the lexer OpenTypeFeatureFile. I've made the old opentypefeature name an alias that can be used in GFM-style code blocks.

Thanks for all the work on getting this ready for merging. It's great to expand the types of files that are supported by Rouge! :)

@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jul 17, 2019
@typemytype
Copy link

Some small side notes:

I understand why you wanted to change to OpenTypeFeatureFile (its indeed in the spec) but it feels also weird: you dont call the lexer for ruby rubyfile

The OpenType feature spec is here: https://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html

AFDKO is a compiler to build binary fonts and OpenType features are a small part of this.

thanks!!

@pyrmont
Copy link
Contributor

pyrmont commented Jul 18, 2019

@typemytype Two points:

  1. Yeah, I struggled with what to do about the name. My criterion was that I was looking for something that made sense to combine with 'language'. 'OpenType Feature File' seemed the most appropriate given that constraint. 'OpenType Feature language' just didn't sound like something people say. Do you have some links to websites where people talk about it?

    That said, I was concerned that it might be a lot to type to syntax highlight GFM-style code blocks and so added 'opentypefeature' to the list of aliases.

  2. I went with the shorter link as part of the string to pass to desc for aesthetic reasons and because that page is so short, I think anyone visiting it would immediately see the spec.

@gferreira
Copy link

@pyrmont people usually call it “fea language” or “fea syntax”. examples:

We will be writing our features in the Adobe OpenType Feature File Syntax (commonly referred to as “.fea”). (source)

Do we have any plan in place to extend the FEA syntax to support OpenType Variation fonts? (source)

OpenTypeFeatureFile doesn’t sound right because, during design and production, the fea code can be scattered across several files, or stored inside fonts… the spec has it backwards, it refers only to the final result.

ps. thanks for your assistance in getting fea syntax supported by Rouge!

@pyrmont
Copy link
Contributor

pyrmont commented Jul 19, 2019

@gferreira Doesn't the example above where it's referred to as 'OpenType Feature File Syntax' demonstrate that this is the appropriate name? To use @typemytype's example from earlier, nobody would say 'Ruby File Syntax'.

@pyrmont
Copy link
Contributor

pyrmont commented Jul 19, 2019

For what it's worth, I should also note that the name of the Ruby class is really just an implementation detail. It doesn't have any impact on how you can use it. The important thing from an end-user perspective is the tag and aliases.

@typemytype
Copy link

its indeed a detail, already super happy to have this support in rouge!

thanks

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.

6 participants