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

[dir_contents] Allow qualified traversal of directories. #1980

Merged
merged 1 commit into from
Mar 28, 2019

Conversation

ejgallego
Copy link
Collaborator

@ejgallego ejgallego commented Mar 26, 2019

This is a cherry-pick of the directory traversal parts from the Dune
PR (#1968) plus a modification to enable the qualified mode inside (include_subdirs ...)

I was uncomfortable with the misnaming done there, so this PR enables
the (include_subdirs qualified) syntax and provides a local part
to the traversal so clients [such as the Coq mode] can use it.

Note that this does introduce a bit of overhead in the tree traversal,
especially in deep trees, it should be possible to optimize if we deem
it necessary.

@ejgallego ejgallego requested review from a user and rgrinberg and removed request for a user March 26, 2019 16:29
@ejgallego ejgallego force-pushed the qualified_traversal branch 2 times, most recently from 82aa456 to 897c780 Compare March 26, 2019 16:31
@ejgallego ejgallego force-pushed the qualified_traversal branch 2 times, most recently from 2de5136 to bb7179c Compare March 27, 2019 16:46
@rgrinberg
Copy link
Member

(include_subdirs qualified|unqalified) Is quite useful for C/Cxx sources. Of course qualified vs. unqualified is meaningless for C, but still splitting your code into different dirs is nice

In retrospect, we should have probably made this stanza a field in the library/exectuable/coqlib stanzas, but whatever.

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

It's good enough to merge. @ejgallego please don't forget to add the parsing error when coq isn't enabled as a project.

@ejgallego
Copy link
Collaborator Author

In retrospect, we should have probably made this stanza a field in the library/exectuable/coqlib stanzas, but whatever.

At first I thought that would make sense, so the qualified part was "local" to the coq stanza; on the other hand at least the recursive nature seems to have a more global scope, so I dunno.

@ejgallego ejgallego force-pushed the qualified_traversal branch from bb7179c to 01dbe95 Compare March 27, 2019 19:00
@ejgallego
Copy link
Collaborator Author

@ejgallego please don't forget to add the parsing error when coq isn't enabled as a project.

Done, thanks! You may want to check the implementation, but I think it is OK.

@ejgallego
Copy link
Collaborator Author

Umm, actually @rgrinberg I am having trouble querying the Dune_project to see if an extension is enabled. I wanted to hook the check here:

    ; "include_subdirs",
      (let+ () = Syntax.since Stanza.syntax (1, 1)
       and+ t =
         let t = ??? in
         let enable_qualified =
           Option.is_some (Dune_project.find_extension_args t Coq.key) in
         Include_subdirs.decode ~enable_qualified
       and+ loc = loc in
       [Include_subdirs (loc, t)])

however this doesn't seem like the right place I'm afraid.

@emillon
Copy link
Collaborator

emillon commented Mar 27, 2019

There's a similar code path when setting up formatting rules, you can have a look at how it works there.

@ejgallego
Copy link
Collaborator Author

Thanks @emillon , I am not sure tho that I should access the super context at parsing time, it seems somehow to complicate things too much, right?

Maybe the parsing check is too much of a trouble if the a Dune_project.t is not in scope @rgrinberg ?

@rgrinberg
Copy link
Member

The dune project is always in scope when parsing. Otherwise, we can't parse as we don't know what extensions are enabled.

You can simply call Dune_project.get_exn () to retrieve it.

@ejgallego ejgallego force-pushed the qualified_traversal branch from 01dbe95 to 5099f2e Compare March 28, 2019 13:38
@ejgallego
Copy link
Collaborator Author

You can simply call Dune_project.get_exn () to retrieve it.

Thanks @rgrinberg , somehow I missed the type on this one.

Rebased; will merge once CI is happy.

This is a cherry-pick of the directory traversal parts from the Dune
PR (ocaml#1968).

I was uncomfortable with the misnaming done there, so this PR enables
the `(include_subdirs qualified)` syntax and provides a `local` part
to the traversal so clients [such as the Coq mode] can use it.

Note that this does introduce a bit of overhead in the tree traversal,
especially in deep trees, it should be possible to optimize if we deem
it necessary.

Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
@ejgallego ejgallego force-pushed the qualified_traversal branch from 5099f2e to f355cb0 Compare March 28, 2019 15:19
@ejgallego ejgallego merged commit 8d601e2 into ocaml:master Mar 28, 2019
@ejgallego ejgallego deleted the qualified_traversal branch March 28, 2019 23:35
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.

4 participants