-
Notifications
You must be signed in to change notification settings - Fork 413
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
refactor(melange): move stanza definition #6775
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,7 +455,7 @@ module Mode_conf = struct | |
; ("native", return @@ Ocaml Native) | ||
; ("best", return @@ Ocaml Best) | ||
; ( "melange" | ||
, Dune_lang.Syntax.since Dune_project.Melange_syntax.t (0, 1) | ||
, Dune_lang.Syntax.since Melange_stanzas.syntax (0, 1) | ||
>>> return Melange ) | ||
] | ||
|
||
|
@@ -2274,7 +2274,6 @@ type Stanza.t += | |
| Cram of Cram_stanza.t | ||
| Generate_sites_module of Generate_sites_module.t | ||
| Plugin of Plugin.t | ||
| Melange_emit of Melange_stanzas.Emit.t | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't this approach more exhaustive from a type checking perspective? What are the advantages of the new approach? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it's equivalent. There's no exhaustiveness checking on open types no matter how you define their constructors.
That we isolate the melange rules from the rest as much as possible. This should make it easy for us to change things without impacting the rest of the code base. |
||
|
||
module Stanzas = struct | ||
type t = Stanza.t list | ||
|
@@ -2391,10 +2390,6 @@ module Stanzas = struct | |
, let+ () = Dune_lang.Syntax.since Section.dune_site_syntax (0, 1) | ||
and+ t = Plugin.decode in | ||
[ Plugin t ] ) | ||
; ( "melange.emit" | ||
, let+ () = Dune_lang.Syntax.since Dune_project.Melange_syntax.t (0, 1) | ||
and+ t = Melange_stanzas.Emit.decode in | ||
[ Melange_emit t ] ) | ||
] | ||
|
||
let () = Dune_project.Lang.register Stanza.syntax stanzas | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to leave just
name
here? The only consumer isMelange_stanzas.syntax
, so maybe it could be moved to that module. Or even inlined insyntax
definition.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I could remove it but it has to stay because it's used to define the dialect for rescript.