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.

Sign up for GitHub

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

Jump to bottom

feat(json): add jsonc to aliases and use it to find parser in markdown embed #4734

Merged
merged 4 commits into from
Jul 7, 2018
Merged

feat(json): add jsonc to aliases and use it to find parser in markdown embed #4734

merged 4 commits into from
Jul 7, 2018

Conversation

ikatyang
Copy link
Member

@ikatyang ikatyang commented Jun 22, 2018 •
edited
Loading

JSON and JSON with Comments are different languages in linguist, but I think it's fine to just add .jsonc to JSON's extensions here since we treat them the same, thoughts?

(Actually I think we should remove those unrelated fields since it's hard to maintain, for example: JSON and JSON with Comments have different linguistLanguageId.) -> #4741

Closes #4611

@j-f1
Copy link
Member

j-f1 commented Jun 22, 2018

Can you add a test with a .jsonc file (as opposed to a Markdown code block)?

@ikatyang
Copy link
Member Author

.jsonc is not an extension now 😅, see github-linguist/linguist@0568239.

@ikatyang ikatyang changed the title feat(json): add jsonc to extensions feat(json): add jsonc to aliases and use it to find parser in markdown embed Jun 22, 2018
suchipi
suchipi approved these changes Jun 22, 2018
View reviewed changes
Copy link
Member

@suchipi suchipi left a comment

Choose a reason for hiding this comment

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

The fields might be useful for integrators, so I don't think we should remove them. I don't think it's the end of the world that jsonc doesn't give the right linguist id in this case.

@ikatyang ikatyang mentioned this pull request Jun 23, 2018
Merged
ikatyang added a commit that referenced this pull request Jun 28, 2018
@ikatyang
feat: update linguist languages (#4741)
0b659b8 
Context: #4734 (comment)

Languages should be easy to maintain now.
@j-f1
Copy link
Member

j-f1 commented Jun 28, 2018

Can you fix the merge conflicts?

@ikatyang
Copy link
Member Author

ikatyang commented Jun 28, 2018 •
edited
Loading

I'm still waiting for github-linguist/linguist#4171 so that I can update linguist-languages then use it here.

(Marked as WIP to avoid confusing.)

@ikatyang ikatyang added the status:wip Pull requests that are a work in progress and should not be merged label Jun 29, 2018
@ikatyang ikatyang removed the status:wip Pull requests that are a work in progress and should not be merged label Jul 6, 2018
@ikatyang
Copy link
Member Author

ikatyang commented Jul 6, 2018

Updated to use JSON with Comments from the latest linguist-languages, ready for the second round review.

@ikatyang ikatyang merged commit 2271992 into prettier:master Jul 7, 2018
@ikatyang ikatyang deleted the feat/json-add-jsonc-extensions branch July 7, 2018 12:46
@ikatyang ikatyang mentioned this pull request Jul 12, 2018
Closed
@ikatyang ikatyang added this to the 1.14 milestone Jul 17, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Oct 15, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@suchipi suchipi suchipi approved these changes

@alexander-akait alexander-akait alexander-akait approved these changes

@j-f1 j-f1 j-f1 approved these changes

Assignees
No one assigned
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Milestone
1.14
Development

Successfully merging this pull request may close these issues.
4 participants
@ikatyang @j-f1 @suchipi @alexander-akait