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 ReScript interface extension '.resi' #6161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

namenu
Copy link

@namenu namenu commented Nov 14, 2022

This PR adds ReScript language's interface files (.resi) in addition to implementation (.res) files.

Description

This PR supplements the original implementation introduced in #4975.

Checklist:

@namenu namenu requested a review from a team as a code owner November 14, 2022 13:53
@glennsl
Copy link

glennsl commented Mar 17, 2023

@lildude As I understand it, the popularity requirement is as described in #5756, normally 2000 files, or 200 files if it's typically a single file per repo. This extension currently has 836 files. However, because of the role this kind of file has, I'm not sure the normal requirements fit this case all that well.. These are interface files, which conceptually have a 1-to-1 relationship with the compilation units ("code files") they describe. But they are also usually implicit, because the file can be omitted if nothing needs to be abstracted away. Therefore there aren't all that many of them, but when they do exist they are really important.

Could I therefore ask that you consider either:

  1. Treat this as a single-file-per-repo extension and use the 200-file requirement instead. Because public repos are mostly libraries, and most libraries are simple enough to only need a single .resi file to describe their public interface.
  2. Include private repositories, if possible, as they tend to have a higher amount of these files since they're more often complex applications that need more internal abstraction than simple libraries. As just one example, the private repo I'm working on has a 1-10 ratio of resi. to .res files (16 of 162, to be precise).
  3. Factor in .res files to get a better sense of language popularity.

@lildude
Copy link
Member

lildude commented Mar 18, 2023

  • Treat this as a single-file-per-repo extension and use the 200-file requirement instead. Because public repos are mostly libraries, and most libraries are simple enough to only need a single .resi file to describe their public interface.

No, as this isn't the case and is bending the "rules" to suit a single use case. This will in turn set a precident which I'll have to argue over and over again.

  • Include private repositories, if possible, as they tend to have a higher amount of these files since they're more often complex applications that need more internal abstraction than simple libraries. As just one example, the private repo I'm working on has a 1-10 ratio of resi. to .res files (16 of 162, to be precise).

Most definitely not. This is an invasion of privacy and contradicts the whole purpose of private repos. Private repos are also private to GitHub staff.

  • Factor in .res files to get a better sense of language popularity.

Linguist doesn't work like that. It looks at files in isolation so we do the same thing for the popularity requirement.

There is already a solution users can, and many do, use: implement an override in the repo like these repos to tell Linguist to treat these files as ReScript.

@glennsl
Copy link

glennsl commented Mar 18, 2023

No, as this isn't the case and is bending the "rules" to suit a single use case.

The referenced issue also says that these are "loose rules", implying that they are intended to be bent to fit cases that don't fit that mold. Though this might still be bending it too far of course.

There is already a solution users can, and many do, use: implement an override in the repo like these repos to tell Linguist to treat these files as ReScript.

Thanks! I did not know about that, and this does neatly solve my immediate issue. 🙏

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