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

Can have Ppx_deriving_json_runtime.Primitives working internally? #19

Open
pedrobslisboa opened this issue Sep 19, 2024 · 5 comments
Open

Comments

@pedrobslisboa
Copy link

pedrobslisboa commented Sep 19, 2024

Description

I'm curious about it. We have to add Ppx_deriving_json_runtime.Primitives for every usage; can we have it internally?

Like (Code just as a sample, not tested or something):

let is_primitive = (* ... *)
let prefix = if (is_primitive) then ("Ppx_deriving_json_runtime.Primitives." ^ prefix) else prefix
let gen_name n = sprintf "%s_%s" prefix n in

Is it to avoid depending on Ppx_deriving_json_runtime.Primitives name, as it can break if the name changes?
What is the motivation?

@pedrobslisboa pedrobslisboa changed the title Can have Ppx_deriving_json_runtime.Primitives working in Can have Ppx_deriving_json_runtime.Primitives working internally? Sep 19, 2024
@davesnx
Copy link
Member

davesnx commented Sep 19, 2024

The only reason I can think of not doing this is to override the primitives such as string/int/..., so I propose to make things rare a bit harder for those use-cases and provide a section on the docs explaining how to do it:

module Ppx_deriving_json_runtime = struct
    include Ppx_deriving_json_runtime
    module Primitives = struct
        include Ppx_deriving_json_runtime.Primitives
        let string = (* ... *)
    end
end

and keep the inlining inside the ppx, as @pedrobslisboa suggests.

@andreypopp
Copy link
Collaborator

I don't think this is a good idea for the reasons stated in the linked ppx_deriving_router issue. Better to stay consistent. If the local open in a module is too annoying, or needs to be spread in multiple modules, can always add -open ... arguments via dune's (flags ...) stanza.

@davesnx
Copy link
Member

davesnx commented Oct 23, 2024

add -open ... arguments via dune's (flags ...) stanza. creates all sort of issues that override to all files in the library, which is worst and I won't consider a valid solution here.

Also, I would do this change on ppx_deriving_router as well.

The entire point of this issue is to make default behaviour simpler, while allow more advanced use-cases more explicit/verbose.

  • The type error that you see when you don't "open" is bad
  • A ppx relying on an open module is a bad practice?

@andreypopp
Copy link
Collaborator

add -open ... arguments via dune's (flags ...) stanza. creates all sort of issues that override to all files in the library, which is worst and I won't consider a valid solution here.

There's a possibility to do open ... per module.

A ppx relying on an open module is a bad practice?

I disagree, all JST ppx work that way.

The entire point of this issue is to make default behaviour simpler, while allow more advanced use-cases more explicit/verbose.

I'm all for making things simpler but what you propose will make things a bit easier, not simpler. Because special casing few types is actually making things more complex and less consistent.

@davesnx
Copy link
Member

davesnx commented Oct 23, 2024

But special casing is shadowing type string, which is... not very common?

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

No branches or pull requests

3 participants