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

Additions to the Perl family of languages #4066

Merged
merged 28 commits into from
Apr 11, 2018
Merged

Conversation

JJ
Copy link
Contributor

@JJ JJ commented Mar 13, 2018

Adding some extensions and sample files for Perl

Description

  • Moves Rexfile to the correct language
  • Adds extensions to Perl and Perl6. Some sample files, too.

Checklist:

JJ added 17 commits October 31, 2015 19:35
And also adding a little bit of text to the README file to help with local use and test.
Refs github-linguist#2309 and also changes github.com to an uniform capitalization.
Initial suggestion by @pchaigno, slightly changed to eliminate false positives such as "classes" or "modules" at the beginning of a line in the =pod

BTW, it would be interesting to just eliminate these areas for language detection.
I just found I had this sitting here, so I might as well follow
instructions to fix it.
@@ -3369,6 +3369,8 @@ Perl:
- ".psgi"
- ".t"
filenames:
- Makefile.PL
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is redundant, because it'll be matched against the *.pl file extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Alhadis That extension is shared between Perl, Perl 6, and Prolog. Adding the Makefile.PL might improve detection for that particular case. Are we sure Makefile.PL is always Perl and not Perl 6 though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I stand corrected, then. 😉 @pchaigno knows more about the Bayesian classifier than I do.

I don't recall ever seeing a Perl 6 version of Makefile.PL... it's always ever been used by Perl 5 modules downloaded/installed from CPAN. I could be wrong though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure. Perl6 has pretty much normalized to .pl6 or .p6 now. It's not using this particular file, either. This is the repo for all modules https://github.com/moritz/perl6-all-modules/search?utf8=%E2%9C%93&q=%22Makefile.PL%22&type= and it's not there even once. Makefile.PL is kind of a Rakefile used for some CPAN distributions; Perl6 uses zef and META6.json

Copy link
Contributor

Choose a reason for hiding this comment

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

@pchaigno knows more about the Bayesian classifier than I do.

I don't know much about the classifier either, but in this particular case, there's a specific filename strategy before we reach the classifier.

PS: Not sure why I keep using "particular case" today...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping... Is there anything missing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it looks fine to me. For the sake of quicker review, it might be better to omit .pod6 since #3366 is more appropriate for discussion its inclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem there is that it might need a bit (or a lot) of work. If you want, you can close that one and I'll redo this one, and then that one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eliminated .pod6. I can try and work on that one, although I can't add or modify that pull request...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't worry about #3366. :) I believe that PR needs formal approval from GitHub staff more than anything.

CONTRIBUTING.md Outdated
@@ -6,11 +6,15 @@ Contributions to this project are [released](https://help.github.com/articles/gi

This project adheres to the [Contributor Covenant Code of Conduct](http://contributor-covenant.org/). By participating, you are expected to uphold this code.

The majority of contributions won't need to touch any Ruby code at all.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you first forked Linguist in November 2015 (!!). You're overwriting unrelated changes that have been made to the docs since then. Please update CONTRIBUTING.md with the latest version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JJ You might want to update your fork...

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 do that 👍

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've restored the HEAD copy. In fact, advice to install CMAKE, which was the only actual change, is already in the README.md file.

@@ -3369,6 +3369,8 @@ Perl:
- ".psgi"
- ".t"
filenames:
- Makefile.PL
- Rexfile
Copy link
Collaborator

Choose a reason for hiding this comment

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

In-the-wild usage is a little questionable... only ~155 results, although they appear to be well-distributed across users/repositories (which technically meets the criteria of "being used in hundreds of repositories"). I think it's okay, particularly since it's an uncommon filename unlikely to cause conflict. @pchaigno, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll defer to @lildude 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem here is that Rexfile was actually in the Perl6 field (my fault: https://github.com/github/linguist/pull/2005/files). It should better be here, Rexfile is in no way Perl6 code.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine. It slipped under the radar before when it was initially added so no point removing it now.

@@ -3387,9 +3389,8 @@ Perl 6:
- ".pl6"
- ".pm"
- ".pm6"
- ".pod6"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was discussed before, and should be omitted. See #3366.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking it out... And I would kindly ask you to reconsider. What brought me (back) here was github/markup#1173, where I spent quite a long time until I discovered .pod6 files were not recognized as Perl6, which had me do a workaround... In fact, I fear .pod6 files will not be recognized as such, even by markup, if that extension is not included here. pod6 is used extensively in https://github.com/perl6/doc, if nowhere else, and as indicated in #3366, it would help adoption of the language. I can wait for that one to be accepted, though... Or just eliminate that change and defer to that one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I'm okay with adding .pod6 as an extension. But since there's been no official consensus on the matter, it might be easier to omit this particular addition and defer to #3366 instead. =)

I see installation of cmake is advised in README.md
@Alhadis Alhadis dismissed their stale review March 28, 2018 10:19

My mistake, never mind.

@JJ
Copy link
Contributor Author

JJ commented Mar 28, 2018 via email

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.

Just a little clean up and we should be good to go.

@@ -0,0 +1,987 @@
=begin pod
Copy link
Member

Choose a reason for hiding this comment

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

This file needs to be removed too as you've removed .pod6 from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JJ
Copy link
Contributor Author

JJ commented Mar 28, 2018 via email

@JJ JJ mentioned this pull request Mar 29, 2018
4 tasks
@lildude
Copy link
Member

lildude commented Apr 6, 2018

Pulling in the latest master into your fork will address the CI failure. You'll also need to address the merge conflict.

@JJ
Copy link
Contributor Author

JJ commented Apr 6, 2018 via email

@JJ
Copy link
Contributor Author

JJ commented Apr 7, 2018

Please check the latest change that addresses conflict.

@JJ
Copy link
Contributor Author

JJ commented Apr 7, 2018

Fixes CI error about alphabetical order on the files.

@JJ
Copy link
Contributor Author

JJ commented Apr 9, 2018

Sorry, what else is left to do here?

@JJ JJ mentioned this pull request Apr 9, 2018
@Alhadis
Copy link
Collaborator

Alhadis commented Apr 9, 2018

Sorry, what else is left to do here?

Wait for this long-ass harvest to finish over my primitive, non-NBN-enabled Australian Internet connection, and be patient.

As an alternative to patience, you could entertain yourself by racing it against the following Perl script and seeing which one finishes in your lifetime:

$_ = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
/a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*[nope]/;

(For anybody reading this who isn't familiar with catastrophic backtracking, the command above will kill your computer)

@JJ
Copy link
Contributor Author

JJ commented Apr 9, 2018

I can do that in Perl 6, where infinity is just another data structure... Or I can wait for the latest CI to finish with flying colors, which it did! 🐧

@Alhadis
Copy link
Collaborator

Alhadis commented Apr 9, 2018

Better idea! Let's all figure out why negative zero exists, and what sick human being thought a zero needed a sign-bit, and wonder why the hell -0 is different to 0 in any fathomable universe.

We can't divide by zero, but we can pointlessly sign it as +0 or -0 instead. Mmmm'kay, IEEE 754, whatever you reckon...

I don't know why I'm flagellating myself thinking about the real-world difference of positive and negative zero when I should be making wisecracks in commit logs like last night.


my $len = 32;
my $this-chromosome = Chromosome.new( chromosome => map( { rand >= 0.5 ?? True !! False }, 1..$len ) );
say $this-chromosome.chromosome();
Copy link
Member

Choose a reason for hiding this comment

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

Is this file somehow uniquely Perl 6 such that it can clearly be differentiated from the exact same code written in Perl 5, or should this be added to the samples/Perl/?

The reason I ask is we use these samples to train the classifier and if this is identical to Perl 5, it's likely to add noise to the Perl 6 side of the classifier when it comes down to a Perl 5 vs Perl 6 comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could also go into tests/fixtures/Perl 6/ if we don't want it as a sample.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a superficial level, it could look similar to Perl. It's been a long time, but if I remember correctly I included it here precisely for that reason. It could go to fixtures alright. Just let me know what to do, and I'll do it straight away.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. If this is just to keep the tests passing, then yup, tests/fixtures/Perl 6/ is definitely the best place for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'm getting some local errors which apparently have nothing to do with moving stuff around. Let me know if there's something I can do about that.

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.

LGTM. Thanks for taking on our feedback so graceously @JJ.

Feel free to open an issue if you need any help sorting out the local errors you mentioned.

@lildude lildude merged commit a9ff59a into github-linguist:master Apr 11, 2018
@JJ
Copy link
Contributor Author

JJ commented Apr 11, 2018

Yay! You made my day. Thanks!

@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