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

Amend Just language entry #6167

Merged
merged 7 commits into from
Feb 17, 2023
Merged

Amend Just language entry #6167

merged 7 commits into from
Feb 17, 2023

Conversation

casey
Copy link
Contributor

@casey casey commented Nov 18, 2022

Sorry for not catching this in the initial review!

  • Remove Justfile alias. just is the name of the tool and the language.
  • Just looks for files called justfile in any case, so add the three most common cases: JUSTFILE, Justfile, and justfile.
  • Add the .just extension, which is in common use. See this search: https://github.com/search?q=extension%3Ajust

- Remove `Justfile` alias. `just` is the name of the tool and the
  language.
- Just looks for files called `justfile` in any case, so add the three
  most common cases: `JUSTFILE`, `Justfile`, and `justfile`.
- Add the `.just` extension, which is in common use
Copy link
Contributor

@mihaigalos mihaigalos left a comment

Choose a reason for hiding this comment

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

@casey, what would you say if we refactor this to Justfile instead of just as file naming?
The way it currently shows up is this:
image

@mihaigalos
Copy link
Contributor

Or capitalize the 1st letter?

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 18, 2022

Or capitalize the 1st letter?

Yes, that's supposed to be the language's human-readable name, not its file extension or interpreter. Note that exceptions are made for non-standard casing if that's how the language's name is canonically spelt (e.g., reStructuredText, mIRC Script, cURL Config, et al).

Remove Justfile alias. just is the name of the tool and the language.

Don't. This is a valid addition: it enables users to highlight Markdown code-blocks with ```justfile as well as ```just.

lib/linguist/languages.yml Outdated Show resolved Hide resolved
@mihaigalos
Copy link
Contributor

@casey: Can you please allow others' contributions to this PR? I wanted to implement some of the suggestions but couldn't since it's read-only.

@casey
Copy link
Contributor Author

casey commented Nov 19, 2022

@mihaigalos I think of the name of the language as being just, as in the just language, and not the justfile language. Doing a quick search on google, "the make language" seems to be more common than "the makefile language", so this is consistent with make. Although, I note that Makefile is used by linguist. Overall I prefer just, and capitalizing it seems to be the convention with linguist, so I changed it to Just.

Don't. This is a valid addition: it enables users to highlight Markdown code-blocks with justfile as well as just.

@Alhadis I'm not sure I see the value of adding an additional alias. Wouldn't it be better to have a single way of annotating markdown code blocks?

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 19, 2022

Wouldn't it be better to have a single way of annotating markdown code blocks?

No, because this is the same lookup machinery responsible for interpreting language: operators in code-searches, or the value of linguist-language= in a .gitattributes file. I think we can all agree that js and javascript should both be mapped to the same language entry. Likewise for Just.

Remember, users who type ```justfile in the middle of a long-winded comment won't always catch themselves writing the "wrong" name. Some might even fail to notice that their annotated code-block is missing highlighting altogether. We should strive for intuitive, "DWIM" behaviour instead of being needlessly restrictive.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 19, 2022

@casey FYI, I pushed a fix to your branch that updated the capitalisation of the samples/just directory—something that's notoriously fiddly to do on a case-insensitive filesystem (so, macOS and Windows). The languages.yml list is also meant to be ordered case-sensitively, so I fixed that as well.

@casey
Copy link
Contributor Author

casey commented Nov 19, 2022

@casey FYI, I pushed a fix to your branch that updated the capitalisation of the samples/just directory—something that's notoriously fiddly to do on a case-insensitive filesystem (so, macOS and Windows). The languages.yml list is also meant to be ordered case-sensitively, so I fixed that as well.

Thanks!

Remember, users who type ```justfile in the middle of a long-winded comment won't always catch themselves writing the "wrong" name. Some might even fail to notice that their annotated code-block is missing highlighting altogether. We should strive for intuitive, "DWIM" behaviour instead of being needlessly restrictive.

Fair enough! I added the alias back.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 19, 2022

Ace. Well, this looks good to me. Now we just need to wait for @lildude to give the GitHub Staffer's Seal-of-Approval™ so we can merge this. :shipit:

@Alhadis Alhadis requested a review from lildude November 19, 2022 05:32
@Alhadis Alhadis changed the title Amend just language entry Amend Just language entry Nov 19, 2022
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
Copy link
Member

lildude commented Nov 30, 2022

Don’t worry about merging in master. I’ll do it again before merging.

@lildude lildude merged commit c1efde2 into github-linguist:master Feb 17, 2023
@casey casey deleted the fix-just branch May 1, 2023 19:24
@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.

5 participants