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

Fixes #4113 : Adds .mir to YAML #4126

Merged
merged 6 commits into from
May 21, 2018
Merged

Conversation

gabru-md
Copy link
Contributor

@gabru-md gabru-md commented May 8, 2018

This Pull request solves issue #4113
It adds .mir in YAML category

Description

It adds .mir in YAML category and this PR also contains a sample code of the extension .mir used in LLVM projects.
The sample is taken from un-official LLVM Mirror on Github.
The sample code is accompanied by LICENSE specified in the repository with the original link to source code also mentioned in comments.

Checklist:

@gabru-md
Copy link
Contributor Author

gabru-md commented May 8, 2018

cc : @pchaigno

@gabru-md
Copy link
Contributor Author

gabru-md commented May 8, 2018

The build here is failing for two possible reasons, that I can understand:

  • The extensions for YAML are not in correct order.
  • According to languages.yml the .mir extension is for more than one language, therefore samples must be provided for both languages bearing the same .mir extension.

I went to the official Mirah community Github to get some examples, but all the files in their repositories have .mirah extensions. I am not sure, but it does seem to me that .mir is not a Mirah extension at all. See the Mirah Files on their github, links are given below

cc : @pchaigno

what should I do? should I remove .mir from Mirah?

Updates :

  • Correcting the order of extensions already present for YAML solved one failure 😅
  • I've removed .mir from Mirah as the extension for Mirah projects is .mirah

@gabru-md
Copy link
Contributor Author

gabru-md commented May 9, 2018

@pchaigno I've got it confirmed that .mir is not an extension for Mirah and hence can be removed from Mirah's extensions in languages.yml.
I have already done that in my Pull Request here, Please review it.
Here is the link to the issue where I got response from Mirah's Owner clarifying the doubt. 👍

@pchaigno
Copy link
Contributor

pchaigno commented May 9, 2018

That file extension dates from the initial versions of Linguist (2011). It's probably fine to remove it given the usage. @lildude ?

@gabru-md
Copy link
Contributor Author

gabru-md commented May 9, 2018

Oh! It has been a bug for so long 😮

@gabru-md
Copy link
Contributor Author

Any reviews on this PR @pchaigno @lildude ? 😅

@RobQuistNL
Copy link
Contributor

From what I understand the first item in the list should always be the main file extension. You can see this is the case for all other file extensions as well. (Example: https://github.com/gabru-md/linguist/blob/a12ce9b736321450108cfdced365ba146a2ffbfb/lib/linguist/languages.yml#L5063)

So the order should not be an issue either. Its probably because of the missing example files.

Please revert .yml to the top, add .mir in an alphabetical order, and then we should be good for this one IMHO :)

@gabru-md
Copy link
Contributor Author

Okay. I'll do it. Wait for travis and then ping you.
👍

@gabru-md
Copy link
Contributor Author

@pchaigno @lildude @RobQuistNL please review :)

@RobQuistNL
Copy link
Contributor

LGTM 👍

@gabru-md
Copy link
Contributor Author

@pchaigno can you provide with a review please.
If there are any changes I should make or it this okay ?

@gabru-md
Copy link
Contributor Author

@lildude @pchaigno can you please review. The PR has been opened for 13 days 😅

@lildude
Copy link
Member

lildude commented May 21, 2018

@lildude @pchaigno can you please review. The PR has been opened for 13 days 😅

Please be patient; Linguist is a hobby for all of the primary contributors and not our jobs. I've been AFK for over a week and I don't review PRs until reviewed by another contributor anyway.

@pchaigno
Copy link
Contributor

I don't review PRs until reviewed by another contributor anyway.

Yep, but on this one, I'm waiting on an answer from you, @lildude :-)

@lildude
Copy link
Member

lildude commented May 21, 2018

Yep, but on this one, I'm waiting on an answer from you, @lildude :-)

Whoops 😊 That's further down my backlog 😄

That file extension dates from the initial versions of Linguist (2011). It's probably fine to remove it given the usage. @lildude ?

Yup. Fine to remove it.

@gabru-md
Copy link
Contributor Author

I'm sorry! I'll be patient. 😅

Please be patient; Linguist is a hobby for all of the primary contributors and not our jobs.

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.

Well then, LGTM!

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 for the PR, welcome to Linguist and sorry it took me so long to get back to this.

@lildude lildude merged commit 6e9bd0a into github-linguist:master May 21, 2018
@gabru-md
Copy link
Contributor Author

Thank you!
I am happy contributing to Linguist.
I will be taking up more issues and help solving them. 🥂

@gabru-md gabru-md deleted the llvm-support branch May 21, 2018 09:05
@pchaigno
Copy link
Contributor

I will be taking up more issues and help solving them. 🥂

We can certainly use the help!

lildude added a commit that referenced this pull request May 30, 2018
@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.

None yet

4 participants