8000 Remove File_path exports. by ceastlund · Pull Request #381 · ocaml-ppx/ppxlib · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove File_path exports. #381

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 19, 2023
Merged

Remove File_path exports. #381

merged 1 commit into from
Jan 19, 2023

Conversation

ceastlund
Copy link
Collaborator

The File_path submodule is tiny, and shadows the File_path library. So I moved its code into Common instead.

ceastlund added a commit to janestreet/ppxlib that referenced this pull request Nov 3, 2022
@ceastlund
Update changelog for ocaml-ppx#381.
861dc2d 
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
@ceastlund ceastlund marked this pull request as ready for review November 3, 2022 22:42
panglesd
panglesd reviewed Dec 21, 2022
View reviewed changes
Copy link
Collaborator
@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

On one hand, I don't like the current status: opening the Ppxlib module brings many values into scope that are most of the time not used; and the doc of the Ppxlib module is hard to understand with so many values.
Moreover, this change seems to break some PPXs: at least this and that.

On the other hand, shadowing the File_path module isn't great when this library is used at the same time as Ppxlib.

Is there something I am missing in favour of this PR, or another solution with less drawbacks?

src/common.mli Outdated
Comment on lines 89 to 91
(** Return the path used in a location. Strips leading "./", uses "" if no location. *)

val get_default_path : Location.t -> string
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doc comment is not associated to the value.

Without ocamlformatting:

Suggested change
(** Return the path used in a location. Strips leading "./", uses "" if no location. *)
val get_default_path : Location.t -> string
(** Return the path used in a location. Strips leading "./", uses "" if no location. *)
val get_default_path : Location.t -> string

@pitag-ha
Copy link
Member

Hey @panglesd,

and the doc of the Ppxlib module is hard to understand with so many val 8000 ues.

That's true. I didn't think about how long/unorganized the Ppxlib module already is when @ceastlund asked whether this change was a reasonable suggestion and I said why not.

Moreover, this change seems to break some PPXs: at least this and that.

@ceastlund will send patch PRs to those two PPXs.

Is there something I am missing in favour of this PR, or another solution with less drawbacks?

I don't think there's anything you're missing: unless I'm missing something as well, this PR is just meant to remove the inconvenience of shadowing the File_path library when opening Ppxlib.

Another solution with less drawbacks in my opinion could be the following:

  • We can move get_default_path to the Location module
  • We can totally remove get_default_path_str and get_default_path_sig. By the end of the day, they are very simple utility functions that aren't even used by anyone. And if someone wants to get the file path of a structure/signature, they could write those 4 lines by hand (or use the file_path component of Expansion_context.code_path, if they're using map_with_context). @ceastlund , do you happen to know why we have those to functions in the first place?

@ceastlund
Copy link
Collaborator Author

No, I don't know why we have these functions. It's possible the best path is to just remove them.

@panglesd
Copy link
Collaborator

@Gopiandcode wrote on discuss:

[...] I’m not sure if this is a very constructive critisism, but in my experience, I’ve found that the API for ppxlib isn’t very “discoverable”:

  • [...]
  • another way this arises is in the large number of modules possible to be opened at any time - it’s easy to fall into a rabbit hole of exploring the APIs modules that are completely irrelevant to most cases of writing a ppx.

I agree that removing File_path and its content seems to be the best option.

@ceastlund
Removed File_path exports.
340e26b 
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
@ceastlund ceastlund force-pushed the remove-file-path-module branch from 861dc2d to 340e26b Compare January 18, 2023 01:45
@ceastlund ceastlund changed the title Moved File_path exports into Common. Remove File_path exports. Jan 18, 2023
@ceastlund
Copy link
Collaborator Author

Pushed a change and retitled the PR. Now removing File_path entirely.

pitag-ha
pitag-ha approved these changes Jan 19, 2023
View reviewed changes
Copy link
Member
@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

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

Sounds good to me. Thanks for the change! Are you going to send the two patch PRs, @ceastlund, or do you want me to do that?

@pitag-ha
Copy link
Member

Btw, just to make sure, I'll ask the ocaml-ci folks whether the CI failure is on the ocaml-ci side (which seems to be the case).

@ceastlund
Copy link
Collaborator Author

I'm going to make those changes to ppx_bench and ppx_inline_test internally at janestreet, and 8000 they'll go out with our next public release.

@ceastlund ceastlund merged commit 4f4f7f7 into ocaml-ppx:main Jan 19, 2023
@pitag-ha
Copy link
Member

Do you know when that release of Jane Street is going to take place? I'm asking because we usually also send patches to the public branches of the PPXs we break given that Jane Street tends to do the public releases not that often.

@panglesd panglesd mentioned this pull request Feb 6, 2023
Merged
panglesd added a commit to panglesd/opam-repository that referenced this pull request Feb 6, 2023
@panglesd
[new release] ppxlib (0.29.0)
f5b1848 
CHANGES:

- Remove `File_path` exports. (ocaml-ppx/ppxlib#381, @ceastlund)

- Add `Ppxlib.Expansion_helpers` with name mangling utilities from ppx_deriving (ocaml-ppx/ppxlib#370, @sim642)

Signed-off-by: Paul-Elliot <peada@free.fr>
panglesd added a commit to panglesd/opam-repository that referenced this pull request Feb 6, 2023
@panglesd
[new release] ppxlib (0.29.0)
ae6aed4 
CHANGES:

- Remove `File_path` exports. (ocaml-ppx/ppxlib#381, @ceastlund)

- Add `Ppxlib.Expansion_helpers` with name mangling utilities from ppx_deriving (ocaml-ppx/ppxlib#370, @sim642)

Signed-off-by: Paul-Elliot <peada@free.fr>
hhugo pushed a commit to hhugo/opam-repository-mingw that referenced this pull request Feb 27, 2023
@panglesd
[new release] ppxlib (0.29.0)
ddb727a 
CHANGES:

- Remove `File_path` exports. (ocaml-ppx/ppxlib#381, @ceastlund)

- Add `Ppxlib.Expansion_helpers` with name mangling utilities from ppx_deriving (ocaml-ppx/ppxlib#370, @sim642)

Signed-off-by: Paul-Elliot <peada@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panglesd panglesd panglesd left review comments

@pitag-ha pitag-ha pitag-ha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ceastlund @pitag-ha @panglesd
0