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 Nushell #6403

Merged
merged 6 commits into from
May 30, 2023
Merged

Add Nushell #6403

merged 6 commits into from
May 30, 2023

Conversation

hustcer
Copy link
Contributor

@hustcer hustcer commented May 4, 2023

Description

Checklist:

@lildude
Copy link
Member

lildude commented May 4, 2023

How is Nushell different from Nu which Linguist already supports and has done so for many many years? From a cursory naïve look, it appears to be the same thing with Nushell often being referred to as Nu, even in the Nushell book.

@hustcer
Copy link
Contributor Author

hustcer commented May 4, 2023

How is Nushell different from Nu which Linguist already supports and has done so for many many years? From a cursory naïve look, it appears to be the same thing with Nushell often being referred to as Nu, even in the Nushell book.

They are totally two different languages with the same name, and they are created by two different teams. That's why I updated lib/linguist/heuristics.yml to distinguish Nushell from Nu using the same extension.

@lildude lildude changed the title Try to Add Nushell Support Add Nushell May 4, 2023
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.

Thanks for the clarification that this is a completely different language.

We need a few changes though:

  1. Please update the template in the OP to state the licenses of the two samples and add a reason for why you have chosen the colour you have.
  2. You need to add the cached license file for the grammar. This should have been created when you run script/add-grammar (you can manually run the command here if you can't find the file).
  3. See inline comments.

test/test_heuristics.rb Show resolved Hide resolved
lib/linguist/languages.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Show resolved Hide resolved
@hustcer
Copy link
Contributor Author

hustcer commented May 4, 2023

  1. Please update the template in the OP to state the licenses of the two samples and add a reason for why you have chosen the colour you have.

I don't know what's OP. I have just updated the description of license for the two samples. And updated the color to #4E9906, that's the main theme color of nushell logo from: https://github.com/nushell

  1. You need to add the cached license file for the grammar. This should have been created when you run script/add-grammar (you can manually run the command here if you can't find the file).

Added

  1. See inline comments.

I have removed .nush extension (It's planed to be used in the future, we can add it after that)
For the other two comments, does that mean the Nushell scripts be recognized as Nu language? We may want to be recognized as Nushell to make it different from Nu

@lildude
Copy link
Member

lildude commented May 4, 2023

I'm showing my age: OP == original post 😁

For the other two comments, does that mean the Nushell scripts be recognized as Nu language? We may want to recognized as Nushell to make it different from Nu

No. We need to ensure that your heuristics do not incorrectly identify Nu files as Nushell.

The heurstics are read top-down. Which means we'll try the Nushell heuristic first and if it doesn't match, we move onto the next regex for that extension. If there isn't one, or there isn't a match, we move onto the classifier. The classifier should be considered the last resort as it's trained on the samples and as such may not be accurate.

By adding - language: Nu to the end of the heuristics, you tell Linguist that if the .nu file it is analysing doesn't match the Nushell regex, it will be identified as Nu and Linguist will then not try analysing the file with the classifier.

The assert_heuristics test you've added needs to be updated to test and confirm this behaviour for the Nu samples we already have. You can see how this is done with other tests in this file.

@hustcer
Copy link
Contributor Author

hustcer commented May 4, 2023

@lildude Thanks for the detailed explanation, I have just updated the PR, Please check it again

@hustcer
Copy link
Contributor Author

hustcer commented May 5, 2023

Should fix #6214

@hustcer hustcer requested a review from lildude May 6, 2023 01:51
@hustcer
Copy link
Contributor Author

hustcer commented May 8, 2023

@lildude Can this PR be Approved?

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.

Note: this PR will not be merged until close to when the next release is made. See here for more details.

@lildude lildude requested a review from a team as a code owner May 30, 2023 09:13
@lildude lildude added this pull request to the merge queue May 30, 2023
Merged via the queue into github-linguist:master with commit 8df79df May 30, 2023
@hustcer hustcer deleted the feature/nushell branch May 30, 2023 10:42
@hustcer hustcer mentioned this pull request Jun 19, 2023
@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.

2 participants