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 TypeSpec #6775

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

timotheeguerin
Copy link

@timotheeguerin timotheeguerin commented Mar 27, 2024

Description

Checklist:

@timotheeguerin timotheeguerin changed the title Typespec new language TypeSpec new language Mar 27, 2024
@lildude
Copy link
Member

lildude commented Mar 28, 2024

I know this PR is still in draft but I thought I'd give you a heads up… you're going to need to add support for "Travelling Salesman Problem" data files (ideally with a heuristic) in this PR too as they share the same extension and there are waaay more of those files on GitHub than appears to be the case for TypeSpec. This is needed to prevent all those files being identified as TypeSpec as Linguist doesn't currently have any languages using the .tsp extension which means this will become the language for all users of that extension if we don't.

@timotheeguerin
Copy link
Author

Thanks for the comment , I was actually not sure if that was only needed if another language was already highlighted.

ill add the heuristic then mark the pr as ready.

@timotheeguerin timotheeguerin marked this pull request as ready for review March 28, 2024 14:31
@timotheeguerin timotheeguerin requested a review from a team as a code owner March 28, 2024 14:31
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.

As TypeSpec will be the only language with the .tsp extension, the heuristic won't be used. This also means all files with a .tsp extension will be identified as TypeSpec.

This is why I said you will need to also add support for the Travelling Salemen Problem data files. This will give Linguist two languages with the .tsp extension and then the heuristic will be used to pick the correct language.

@timotheeguerin
Copy link
Author

ah sorry misread that, will look into this

@timotheeguerin
Copy link
Author

Hi @lildude doing a little bit of research on those Travelling Salemen Problem data files, there doesn't seem to be any existing grammar best I can find is this specification http://comopt.ifi.uni-heidelberg.de/software/TSPLIB95/tsp95.pdf, wondering here what bar you are looking for? Would adding some basic grammar for that file format(key, values, punctuation) be consider good and would allow us to add both language to linguist?
This is definitely a decent amount of work for a format we do not know and could be likely to specify incorrectly in the grammar so just trying to see what will allow us to move forward.

@lildude
Copy link
Member

lildude commented Mar 28, 2024

There's no need for a grammar if you can't find one. One can always be added later.

@timotheeguerin
Copy link
Author

Thanks for the clarification, should I then follow the steps to add a new language but skip the script/add-grammar step for this new one?

  • add entry in languages.yml
  • update id
  • add samples?
  • pick a color?
  • add heuristic to disambiguate

@lildude
Copy link
Member

lildude commented Apr 2, 2024

Thanks for the clarification, should I then follow the steps to add a new language but skip the script/add-grammar step for this new one?

Yes, though don't worry about the colour. We can fallback to the default for now.

And two samples of each should be sufficient for the classifier.

As an aside, TypeSpec isn't popular enough for inclusion right now, however it might be when I come to merge this PR which will only happen just before I make the next release. I review usage for all pending PRs at the time. I aim to make a new release approximately every 3-4 months inline with our Enterprise Server release cycle.

@lildude lildude changed the title TypeSpec new language Add TypeSpec Apr 2, 2024
lib/linguist/languages.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
timotheeguerin and others added 3 commits April 3, 2024 16:36
Co-authored-by: John Gardner <gardnerjohng@gmail.com>
Co-authored-by: John Gardner <gardnerjohng@gmail.com>
@Alhadis
Copy link
Collaborator

Alhadis commented Apr 4, 2024

@timotheeguerin I tried pushing some fixes to your branch (to resolve the various error failures, most of which were pedantic in nature), but I don't have write access:

λ GitHub-Linguist (typespec-fixes): git push -u fork typespec-fixes
Enumerating objects: 17, done.
Counting objects: 100% (17/17), done.
Delta compression using up to 16 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 892 bytes | 892.00 KiB/s, done.
Total 9 (delta 7), reused 0 (delta 0), pack-reused 0 (from 0)
remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
To github.com:timotheeguerin/linguist.git
 ! [remote rejected]   typespec-fixes -> typespec-fixes (permission denied)
error: failed to push some refs to 'github.com:timotheeguerin/linguist.git'

I've sent you a pull-request to get the changes merged instead.

Copy link
Collaborator

@Alhadis Alhadis left a comment

Choose a reason for hiding this comment

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

Approving the change-set I requested earlier; note that this is exclusive of fixes I submitted to your fork earlier. See timotheeguerin/linguist#1.

@timotheeguerin
Copy link
Author

@Alhadis thanks for the fixes, I think I merged your PR correctly let me know if I didn't. For the permission issue i do have this option on in the PR, is there something else I should be doing to give access
image

@Alhadis
Copy link
Collaborator

Alhadis commented Apr 9, 2024

thanks for the fixes, I think I merged your PR correctly

Yup, just synced my checkout's branch with yours, and the tests are all passing with flying colours. 👍

For the permission issue i do have this option on in the PR, is there something else I should be doing to give access

No, uh, that should be sufficient. Unless I was removed as a maintainer sometime in the last year (probably due to my break in activity), I should've been able to push changes to this PR directly. 😕🤔 Not sure… 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants