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 extensions for nim files #4295

Merged
merged 6 commits into from
Nov 7, 2018
Merged

Conversation

timotheecour
Copy link
Contributor

what's changing

this fixes https://forum.nim-lang.org/t/4307: syntax highlight of nims, nim.cfg, nimble files broken on github

as reported there, these don't syntax highlight correctly:

Checklist:

Copy link

@alehander92 alehander92 left a comment

Choose a reason for hiding this comment

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

looks ok, is .nimrod still used?

@timotheecour
Copy link
Contributor Author

only old projects

Copy link
Contributor

@pchaigno pchaigno 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 your contribution!

I left a few comments inline regarding new extensions and filenames. Could you also add the sample files (including sample files for new filenames) under samples/Nims/?

For other reviewers, here what I found for extension/filename:

  • extension:nimble, 1776 repos, 644 users
  • extension:nims defined, 52 files
  • filename:nim.cfg, 694 repos, 362 users
  • filename:config.nims, 146 files

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

@pchaigno

Could you also add the sample files (including sample files for new filenames) under samples/Nims/

done

  • I'm assuming you meant samples/Nim/, not samples/Nims/

  • /cc @Araq for these samples, I took some files from Nim repo, let me know if that's not ok (I added from .url... at top-level for each such file)

  • @pchaigno this should be added to readme (the fact we need to add to samples), as well as to pull request template (IIRC it wasn't mentioned?)

Copy link

@kaushalmodi kaushalmodi left a comment

Choose a reason for hiding this comment

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

Looks great to me.

@Araq
Copy link

Araq commented Oct 16, 2018

@timotheecour That's totally fine.

@timotheecour
Copy link
Contributor Author

timotheecour commented Oct 25, 2018

/cc @pchaigno friendly ping; anything else that needs changing?

@timotheecour
Copy link
Contributor Author

/cc @pchaigno @Alhadis @lildude
ping (no feedback, good to merge?)

@pchaigno
Copy link
Contributor

pchaigno commented Nov 6, 2018

  • I'm assuming you meant samples/Nim/, not samples/Nims/

Yes, sorry.

  • @pchaigno this should be added to readme (the fact we need to add to samples), as well as to pull request template (IIRC it wasn't mentioned?)

It is mentioned in the contribution guidelines. I'll change the PR template to explicitly mention the directory.

Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @timotheecour. I wanted to recheck the usage.

Two comments to address below, but after that, I'd say we're good to merge!

samples/Nim/nim.cfg Outdated Show resolved Hide resolved
lib/linguist/languages.yml Show resolved Hide resolved
@timotheecour
Copy link
Contributor Author

@pchaigno PTAL

Copy link
Contributor

@pchaigno pchaigno 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 your time and contribution @timotheecour!

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 and welcome to Linguist. Thanks for your contribution.

@lildude lildude merged commit 21005b3 into github-linguist:master Nov 7, 2018
@timotheecour
Copy link
Contributor Author

@pchaigno

not sure I'm understanding the result:
https://github.com/github/linguist/search?q=nim&unscoped_q=nim

image

@pchaigno
Copy link
Contributor

pchaigno commented Nov 7, 2018

@timotheecour These changes will only apply once a new release of Linguist is cut.

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.

6 participants