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 Pod 6 to the languages.yml #4083

Merged
merged 20 commits into from
May 2, 2018
Merged

Conversation

Tyil
Copy link
Contributor

@Tyil Tyil commented Mar 28, 2018

Add Pod6 as a language to languages.yml.

Description

There's been a long standing issue in the Perl 6 repos to have pod files rendered properly: github/markup#907. I'm trying to solve this issue, and while working through https://github.com/github/markup (going by instructions from github/markup#907 (comment)) I found it using Linguist, which did not report any language back for a .pod6 file.

GitHub not rendering .pod6 files correctly has stopped me from using it altogether for READMEs, but I'd like this to change.

I've been reading through the CONTRIBUTING file, but adding the Grammar I wanted to try with (https://github.com/perl6/atom-language-perl6/blob/master/grammars/perl6fe.cson) gives the error it already exists. (I'm not sure if this Grammar will work correctly, but I can try to make a grammar for this specific purpose if needs be).

Additionally, the real-world samples (like this one) I know off come from Artistic-2.0 licensed repositories. This license is not in the whitelisted licenses list. Can this license be added, or should I write one myself?

Checklist:

  • I am associating a language with a new file extension.
    • The new extension is used in hundreds of repositories on GitHub.com
    • I have included a real-world usage sample for all extensions added in this PR:
      • Sample source(s):
        • [URL to each sample source, if applicable]
      • Sample license(s):
    • I have included a change to the heuristics to distinguish my language from others using the same extension.

@JJ
Copy link
Contributor

JJ commented Mar 29, 2018

As an answer to #4066 , we had better be focusing on #3366, created by @samcv

@lildude
Copy link
Member

lildude commented Apr 6, 2018

Pulling in the latest master into your fork will address the CI failure. It's likely to then highlight a few more things that need addressing in this PR 😉 .

@@ -3458,6 +3458,14 @@ Pod:
- perl
tm_scope: none
language_id: 288
Pod6:
type: prose
ace_mode: perl6
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add this line to enable Perl/Pod 6 highlighting:

+  tm_scope: source.perl6fe

Perl 6's grammar includes support for Pod6 markup, judging by the comments and pattern blocks.

@Alhadis
Copy link
Collaborator

Alhadis commented Apr 6, 2018

@JJ Should the entry's name be spelt Pod6 or Pod 6, with a separating space? The latter is the conventional spelling used for Perl 6 (and Perl 5...), so I'm inclined to think a space should be used.

However, I'm only familiar with Perl 5 and Perl 6 and its idioms are a mystery to me (aside the whole s/camel/butterfly/ thing 😉). You likely know more than I do.

@lildude
Copy link
Member

lildude commented Apr 6, 2018

Should the entry's name be spelt Pod6 or Pod 6, with a separating space?

Ah, good question. I'd rather get this addressed before merging this PR as changing it after the event is a pain in the 🍑 on the GitHub side of things as it requires a database migration... don't ask 😞

@JJ
Copy link
Contributor

JJ commented Apr 6, 2018 via email

@JJ
Copy link
Contributor

JJ commented Apr 6, 2018

Anyway, you got me a little confused. Weren't we gonna focus on #3366?

@Alhadis
Copy link
Collaborator

Alhadis commented Apr 6, 2018

Weren't we gonna focus on #3366?

That was my idea, but it's @lildude's prerogative how we go about this.

@JJ
Copy link
Contributor

JJ commented Apr 6, 2018 via email

@samcv
Copy link
Contributor

samcv commented Apr 6, 2018

I've updated my PR to merge cleanly, and passes tests #3366 If we have decided we want Pod 6 to be its own language, then I will adjust it for that. At the moment it classifies .pod using Pod 6 as Perl 6, but that was because previously the view was there was not enough Pod6 usage to make it its own language. @lildude what are you thoughts on this?

@JJ
Copy link
Contributor

JJ commented Apr 7, 2018

Problem is Pod 6 is Perl 6, it's part of the language specification and it's interpreted or compiled by the very same program. Having them as separate languages might have some implications, like having some heavily documented modules maybe interpreted as Pod 6 instead of Perl 6. The situation is slightly different in Perl, where Pod is interpreted by Perl as "I don't care, this is a comment" with the actual parsing referred to the Pod parser. Perl 6 says "OK, this is a comment, but one I should care about" and puts the structured AST in the structure of the program, to be then retrieved and interpreted any way you want...

@samcv
Copy link
Contributor

samcv commented Apr 7, 2018

@JJ in my PR it does detection on .pod files to see if they're pod6 or not. And it doesn't attempt to classify anything that is .pl or .pl6 as pod, which I think works around that. Though that could be an issue if we tried to distinguish .pl6/.pl files and try and tell if they should be labeled pod6…

@JJ
Copy link
Contributor

JJ commented Apr 7, 2018 via email

if /^\s*=\w+$/.match(data)
if /^=pod|=cut/.match(data)
Language["Pod"]
elsif /^\s*=begin pod/.match(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth testing Perl 6 against /^\s+=(begin|cut|pod)/ instead (note the use of \s+).

To my understanding, Perl 6 allows whitespace to precede Pod lines, whereas Perl 5 doesn't. Testing for leading whitespace would be a good disambiguation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use your suggested regex.

@lildude
Copy link
Member

lildude commented Apr 9, 2018

Started a discussion of the "Perl 6" vs "Pod 6" decision over at #3366 (comment) to keep things simple and in one place.

@Tyil
Copy link
Contributor Author

Tyil commented Apr 13, 2018

@lildude From #3366 (comment) I understand preference goes out toward this PR. Is there anything I can do to improve the PR at this point in order to get it merged?

@lildude
Copy link
Member

lildude commented Apr 16, 2018

@lildude From #3366 (comment) I understand preference goes out toward this PR. Is there anything I can do to improve the PR at this point in order to get it merged?

Yup, please remove the .pod6 extension. This extension is not popular enough for inclusion just yet. Once this PR lands, peeps can always create a manual override for the .pod6 ext until such time as popularity does increase to the point we can add back the extension.

@Tyil
Copy link
Contributor Author

Tyil commented Apr 24, 2018

I've added the Cookbook.pod as a sample file and updated the heuristics.rb for Pod 6. Let's see if Travis thinks this'll work.

@Grinnz
Copy link

Grinnz commented Apr 24, 2018

Are Pod 6 files always going to have a single word directive either? I feel like the main heuristics only being applied if a single word directive is found is a problem; is there a reason for requiring that?

Copy link
Contributor

@samcv samcv left a comment

Choose a reason for hiding this comment

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

Made some suggestions. Also @Grinnz, I too think that should be changed, since most Perl 6 pod will use multiple word declarations.

@@ -380,6 +380,24 @@ def call(data)
end
end

disambiguate ".pod" do |data|
if /^[\s&&[^\n]]*=\w+$/.match(data)
if /^[\s&&[^\n]]+=(begin|cut|pod)/.match(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perl 6 pod does not use =cut or =pod at all so those should be removed from this match. In addition, all =begin blocks in Perl 6 require something coming after it, so this seems redundant with the check below?

Copy link

@Grinnz Grinnz Apr 24, 2018

Choose a reason for hiding this comment

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

Maybe it could be changed to (begin|head) or a set of more common indented directives?

if /^[\s&&[^\n]]*=\w+$/.match(data)
if /^[\s&&[^\n]]+=(begin|cut|pod)/.match(data)
Language["Pod 6"]
elsif /^=(pod|cut)/.match(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe allow it to include spaces before =pod or =cut in case the document is formatted improperly? Optional, since that's not actual valid Pod, but could occur on accident.

Copy link

Choose a reason for hiding this comment

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

IMO if someone indents a Perl 5 pod directive in a .pod file that's their own fault, it doesn't need to complicate this heuristic.

Language["Pod 6"]
elsif /^=(pod|cut)/.match(data)
Language["Pod"]
elsif /^[\s&&[^\n]]*=(comment|begin pod)/.match(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add =begin para. Most cases a Perl 6 pod file will have =begin pod, but that is not needed if blocks certain blocks are used which don't need =begin pod. Many blocks can be used this way, but if it's documentation then =begin para is almost certain to be used to allow writing paragraphs. Unlike the case when the user does =begin pod in which text is processed not as Perl 6 code but as a string, if =begin pod is used then they well need some way to write any text that is not a =head title (causing them to almost certainly use =begin para).

@Tyil Tyil changed the title Add pod6 to the languages.yml Add Pod 6 to the languages.yml Apr 24, 2018
@samcv
Copy link
Contributor

samcv commented Apr 24, 2018

@Tyil Another thing unique to Perl 6 is =item1, =item2 etc. We still have =item, which is an alias for =item1. So if the pod document has item and some number of digits, then it is Pod 6

@Grinnz
Copy link

Grinnz commented Apr 24, 2018

LGTM now, other than if you also want to recognize any indented-only directives as Pod 6 like was there before, I don't know what directives are commonly indented. If this will catch most Pod 6 as it is then 👍 (and hope that github will recognize .pod6 soon).

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

This is looking much nicer now than it was at one stage. Thanks for the PR and your patience. Welcome to Linguist.

@lildude lildude merged commit df393a0 into github-linguist:master May 2, 2018
@Tyil Tyil deleted the add-pod6 branch May 2, 2018 12:04
@lildude
Copy link
Member

lildude commented May 30, 2018

Unfortunately, I'm going to have to revert this and open a new PR for it for the moment as it requires Pod 6 rendering support to be implemented in github/markup#1173 or github/markup#1185 and GitHub.com updated. This needs to coincide with the release of a Linguist release that introduces support for Pod 6 so we don't end up leaving people scratching their heads trying to work out why one pod document is being rendered and not another.

I don't currently have the bandwidth to look into the markup side of things either.

lildude added a commit that referenced this pull request May 30, 2018
lildude added a commit that referenced this pull request May 30, 2018
lildude added a commit that referenced this pull request May 30, 2018
lildude added a commit that referenced this pull request May 30, 2018
lildude added a commit that referenced this pull request Dec 10, 2018
* Revert "Revert "Add Pod 6 to the languages.yml (#4083)" (#4148)"

This reverts commit e9ad913.

* add .pod6 to Pod 6 extensions

* bump

* remove XPM disambig from .pod
@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.

6 participants