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 support for font-specific formats #3142

Merged
merged 17 commits into from
Dec 6, 2016
Merged

Add support for font-specific formats #3142

merged 17 commits into from
Dec 6, 2016

Conversation

Crissov
Copy link
Contributor

@Crissov Crissov commented Aug 4, 2016

@@ -2668,6 +2669,16 @@ OpenSCAD:
tm_scope: none
ace_mode: scad

OpenType Font Feature:
type: programming
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this PR is accepted, this should be data, not programming. Same with Spline Font Data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll agree that data is appropriate for .sfd, but not so much for .fea

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's convinced you that OpenType features are a "programming language"...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, maybe it’s markup (like Postscript and CSS) but OT features are used to implement substitution algorithms etc. That sounds like programming to me. It’s certainly not just data like an image or config file.

Copy link
Collaborator

@Alhadis Alhadis Aug 4, 2016

Choose a reason for hiding this comment

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

PostScript? Uh, no, PostScript is a fully Turing-complete programming language. CSS, though... yes, that's markup.

I'm well aware of what OpenType features are, but I'd argue they're no closer to programming than SVG source is. They describe coordinates, geometry and hinting for the rendering environment...

Copy link
Contributor Author

@Crissov Crissov Aug 4, 2016

Choose a reason for hiding this comment

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

I know that PS is TC and so is TeX, but like CSS they both have type: markup in languages.yml. CSS is a declarative language (and doesn’t mark up text)), but so are functional programming languages … CSS is certainly not a markup language like HTML is.

Copy link
Collaborator

@Alhadis Alhadis Aug 4, 2016

Choose a reason for hiding this comment

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

The hell. I didn't even notice PostScript was classed as markup, and I was even editing that part the other night.

Right, time to amend.

EDIT: Done.

Crissov added 2 commits August 4, 2016 16:23
`type: data` for SFD
OpenType feature ← type: markup
@Alhadis
Copy link
Collaborator

Alhadis commented Aug 4, 2016

You'll need to fix the order of file extensions. From the build's feedback:

TestPedantic#test_nonprimary_extensions_are_sorted [/home/travis/build/github/linguist/test/test_pedantic.rb:15]:
.mkdown should come after .md.txt

alphabetic order
@pchaigno
Copy link
Contributor

pchaigno commented Aug 5, 2016

@Crissov Thanks for the pull request! Please see the guidelines to add support for a new extension to Linguist to complete the additional steps required.

@Alhadis
Copy link
Collaborator

Alhadis commented Oct 27, 2016

@Crissov You can use this package to provide syntax highlighting for OpenType feature files and spline-font databases. The relevant scopes are:

  • OpenType feature file: source.opentype
  • SFD: text.sfd

Also, it appears the "proper" name for the former is "OpenType feature file", not "OpenType font feature". That's going by how the spec refers to the format.

You'll also need to change OpenType's language type to data, as previous suggested. It's not markup.

incorporated suggestions:

- “OpenType Font Feature” → “Opentype feature” (no “file” at the end, because that’s left out almost everywhere else, too)
- `tm_scope` according to https://github.com/Alhadis/language-fontforge
@Crissov Crissov changed the title markdown and font-specific updates languages.yml font-specific updates languages.yml Oct 27, 2016
- `.md.txt` Markdown
- `.text` Plain Text
- .fea
#- .otl (rare)
#interpreters:
#- makeotf
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure I correctly understand the purpose of interpreters

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's used for identifying languages based on an interpreter directive. For example, we have node listed as an interpreter for JavaScript, which tells Linguist that #!/usr/bin/env node identifies the file as being that language.

Since OpenType feature files aren't executed, it's safe to leave this out. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks

remove comment
@Alhadis Alhadis self-assigned this Nov 13, 2016
@Alhadis
Copy link
Collaborator

Alhadis commented Nov 13, 2016

Sorry about the long wait! Didn't mean to take this long to get back to you. 😞

Alright, great news: all three additions check out.


.sfd

Bit surprised by the abundance of these files; I hadn't expected FontForge's .sfd format was so well-distributed, heh. 11,018 files on GitHub, hosted among 880 users among 1,042 unique repositories. Definitely worth inclusion.

Each of the .sfd files harvested from search results turned out to be a Spline Font Database, so I don't predict there'll be any files misclassified here.

.fea

These are slightly less distributed than .sfd files, but still too abundant to ignore: 14,236 indexed results, and among the 2,695 files I downloaded, half of which were duplicates, but distributed between 278 users among 404 unique repositories.

FONTLOG

Can't really go wrong here... 17,730 results, each with consistently similar content and structure.


Now, you'll need to fix a few things before we can merge this:

  1. Pull/merge the latest changes from master into your branch
  2. Rerun script/set-language-ids --update to fix missing/conflicting language IDs
  3. Tweak each entry's name in languages.yml

/cc @pchaigno, @larsbrinkhoff

@@ -2668,6 +2668,15 @@ OpenSCAD:
tm_scope: none
ace_mode: scad

Opentype feature:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be changed to OpenType Feature File (case-sensitive, please).

@@ -3635,6 +3644,13 @@ SourcePawn:
tm_scope: source.sp
ace_mode: text

Spline Font Data:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The canonical name for the format is Spline Font Database, which is what this needs to be. Again, please make sure it's capitalised correctly (the way I've written it here).

Copy link
Collaborator

@Alhadis Alhadis left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 13, 2016

@Crissov Uhm, you've... kind of committed the conflicts you were supposed to fix. The affected regions are lines 2956-2969 and 4041-4052. Just erase the the blank lines and <<<<====>>>> junk, and rerun script/set-language-ids --update.

The diff for languages.yml is too large to open on GitHub, so I can't comment on the conflicted areas directly.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 15, 2016

@Crissov Just paging you with a reminder we're still waiting for the conflicts to be fixed before we can finish our review. =)

@brndnblck
Copy link

@Crissov looks like this is near the finish line, but you have some additional conflicts to resolve.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 18, 2016

@Crissov I sense you may be having difficulties, so I sent you a pull request to resolve these conflicts.

@Alhadis Alhadis changed the title font-specific updates languages.yml Add support for font-specific formats Nov 19, 2016
@Alhadis
Copy link
Collaborator

Alhadis commented Nov 19, 2016

Alright, there're two build failures, but only one of them we can fix (the second failure isn't your fault; it's currently a known issue).

This is the one we still have to fix:

  1) Failure:
TestLanguage#test_all_languages_have_grammars [/home/travis/build/github/linguist/test/test_language.rb:409]:
The following languages' scopes are not listed in grammars.yml. Please add grammars for all new languages.
If no grammar exists for a language, mark the language with `tm_scope: none` in lib/linguist/languages.yml.
OpenType Feature File source.opentype
Spline Font Database  text.sfd

I admit the error here was probably mine, since I hastily deleted my fork before seeing what the build result was. Silly me.

@Crissov Try these steps:

  1. Clone your fork to a new directory on your system.
  2. Checkout your branch patch-2
  3. Run script/bootstrap to install the necessary submodules
  4. Run the following:
perl -pi -e 's|script\K (?=list-grammars)|/|;' script/add-grammar
script/add-grammar Alhadis/language-fontforge
git add --all && git commit -m "Update submodules and fix grammar-list script"

Then push to your fork as you would normally. Note this should be done in a clean repository to eliminate the risk of unregistered submodules complicating things. There've been quite a few replaced over the course of this year, and since your fork is a little dated, it's quite likely you have stray submodules floating around...

Using script/add-grammar is the simplest fix here, but the current state of the script isn't working due an error in 43fa563. I've added an extra step that fixes it in order to allow the script to run.

Probably not a clean way to do it, but hey.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 23, 2016

@Crissov I've sent you a second pull request in the hopes of closing the book on this.

@pchaigno Tests were still failing locally for the Ant submodule (even after merging master into the fork's branch). Could this be related to licensee somehow?

@pchaigno
Copy link
Contributor

@Alhadis You'll need to update dependencies: bundle update.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 24, 2016

Ah, right. My understanding of what bundle even is is still really shaky.

Is it because I'm from the land of JavaScript, not Ruby? ;)

Actually, serious question: shouldn't there be a version check or something to ensure dependencies are up to date? Because I had no idea what was going on.

@pchaigno
Copy link
Contributor

Is it because I'm from the land of JavaScript, not Ruby? ;)

I'm not sure I get it myself, but then I'm not sure either of which land I'm from :p

Actually, serious question: shouldn't there be a version check or something to ensure dependencies are up to date? Because I had no idea what was going on.

We could increase the version of Licensee needed in github-linguist.gemspec. I'm a little hesitant to do that because Licensee 8.7.0 isn't actually needed for anything else than tests.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 26, 2016

@Crissov Could you please merge the PR so we can get this wrapped up?

Add missing grammar to submodules list
@Alhadis
Copy link
Collaborator

Alhadis commented Dec 2, 2016

@brandonblack This looks merge-ready to me. :shipit: ?

Please also remember to squash-merge; the PR's commit history is extremely convoluted and would pollute the codebase's revision history.

@brndnblck brndnblck merged commit ecdae83 into github-linguist:master Dec 6, 2016
@brndnblck
Copy link

Thanks @Crissov!

@Crissov
Copy link
Contributor Author

Crissov commented Dec 6, 2016

It was really @Alhadis who did all the nasty work.

@Crissov Crissov deleted the patch-2 branch December 6, 2016 20:26
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants