-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
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 😉 . |
lib/linguist/languages.yml
Outdated
@@ -3458,6 +3458,14 @@ Pod: | |||
- perl | |||
tm_scope: none | |||
language_id: 288 | |||
Pod6: | |||
type: prose | |||
ace_mode: perl6 |
There was a problem hiding this comment.
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.
@JJ Should the entry's name be spelt However, I'm only familiar with Perl 5 and Perl 6 and its idioms are a mystery to me (aside the whole |
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 😞 |
The canonical answer is: neither. It should probably be Pod<non-breaking
whitespace>6. But I'd better ask. Please hold on while I get some answer.
|
Anyway, you got me a little confused. Weren't we gonna focus on #3366? |
OK. Still trying to find out the answer.
|
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? |
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... |
@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… |
That's the thing, there are several heuristics taking place and sometimes
you don't really know which one is actually calling the shots. Besides, if
you look at it from a principled point of view, Pod 6 is simply not a
different language.
I came here from the other side, the one that wants GitHub to render pod6.
So I don't really know. I'm just happy if the PR is accepted, whatever way
you want to call it. It would open the door for having GitHub render .pod6,
which was my main objective all along. Thanks for your work here.
|
lib/linguist/heuristics.rb
Outdated
if /^\s*=\w+$/.match(data) | ||
if /^=pod|=cut/.match(data) | ||
Language["Pod"] | ||
elsif /^\s*=begin pod/.match(data) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Started a discussion of the "Perl 6" vs "Pod 6" decision over at #3366 (comment) to keep things simple and in one place. |
@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 |
I've added the |
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? |
There was a problem hiding this 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.
lib/linguist/heuristics.rb
Outdated
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
lib/linguist/heuristics.rb
Outdated
if /^[\s&&[^\n]]*=\w+$/.match(data) | ||
if /^[\s&&[^\n]]+=(begin|cut|pod)/.match(data) | ||
Language["Pod 6"] | ||
elsif /^=(pod|cut)/.match(data) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/linguist/heuristics.rb
Outdated
Language["Pod 6"] | ||
elsif /^=(pod|cut)/.match(data) | ||
Language["Pod"] | ||
elsif /^[\s&&[^\n]]*=(comment|begin pod)/.match(data) |
There was a problem hiding this comment.
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 Another thing unique to Perl 6 is |
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). |
There was a problem hiding this 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.
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. |
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: