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

feat: add support to extension .res, .resi, .rei #53

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

helderberto
Copy link
Contributor

It will enable reason-mode to .res to ReScript

@Khady
Copy link
Collaborator

Khady commented Sep 25, 2020

There are no resi files in rescript?

I'm not sure it is a good idea to have this merged. The syntax of rescript is different from the syntax of rescript. And the rescript people want to have full control of the pipeline which I guess mean they will have their own emacs mode.

@Khady
Copy link
Collaborator

Khady commented Sep 25, 2020

https://rescript-lang.org/docs/manual/latest/module#every-resi-file-is-a-signature

if we want to merge this, it must be enabled on resi files too.

@helderberto helderberto changed the title feat: add support to extension .res feat: add support to extension .res, .resi, .rei Sep 25, 2020
@helderberto
Copy link
Contributor Author

https://rescript-lang.org/docs/manual/latest/module#every-resi-file-is-a-signature

if we want to merge this, it must be enabled on resi files too.

Updated to accept .resi, can u check?

reason-mode.el Outdated
@@ -233,7 +233,7 @@ Argument END marks the end of the function."
(setq-local parse-sexp-lookup-properties t))

;;;###autoload
(add-to-list 'auto-mode-alist '("\\.rei?\\'" . reason-mode))
(add-to-list 'auto-mode-alist '("\\.\\(res\\(i\\)?\\|rei\\)$" . reason-mode))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(add-to-list 'auto-mode-alist '("\\.\\(res\\(i\\)?\\|rei\\)$" . reason-mode))
(add-to-list 'auto-mode-alist '("\\.\\(resi?\\|rei?\\)$" . reason-mode))
Suggested change
(add-to-list 'auto-mode-alist '("\\.\\(res\\(i\\)?\\|rei\\)$" . reason-mode))
(add-to-list 'auto-mode-alist '("\\.res?i?$" . reason-mode))

your commit disables the mode for re file if I undersand it correctly. And I think it can be simplified. But I haven't tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I updated the regex to support the commented files.

It enables reason-mode with the following file extensions:
- .re
- .rei
- .res
- .resi
@helderberto
Copy link
Contributor Author

Hey @Khady, what do you think?

@Khady
Copy link
Collaborator

Khady commented Sep 29, 2020

My problem with merging this is that rescript syntax is the not the same reason syntax. So the syntax provided by this mode is not fully relevant. Also if people are using merlin/ocaml-lsp based on this mode it will also not work given the different syntax. Maybe it is better to have a different mode for rescript?

I am not against merging this, I guess it can help at least temporarily. Just not sure that it is the right thing to do.

@helderberto
Copy link
Contributor Author

helderberto commented Sep 29, 2020

I agree with you about rescript-mode will make more sense.

But, like you said it's temporary.

Wht do you think about we create a rescript mode? And just to start use the same code as reason-mode.

If you add me to the reason-editor community I can work on this.

@Khady
Copy link
Collaborator

Khady commented Sep 29, 2020

I don't have the rights to add you to the organization. You should probably discuss the rescript mode on the rescript forum and they will guide you on where to host it. It can totally be a fork of the reason-mode to start.

In the meantime I'll merge this PR as it is better than nothing. Thanks for submitting it. Please let me know if/when it is no longer necessary.

@Khady Khady merged commit 5690544 into reasonml-editor:master Sep 29, 2020
@helderberto helderberto deleted the feat/add-support-rescript branch September 29, 2020 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants