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

defxdoc :code-source definitional support #1459

Open
wants to merge 3 commits into
base: testing
Choose a base branch
from

Conversation

bendyarm
Copy link
Member

Adds the :code-source argument to defxdoc and defxdoc+.
There is an example of its use in ubi-doc.lisp.
Does not yet push the value into the doc entry and does not yet handle rendering.
See also a few inline comments in the code and in this PR.

@@ -8,6 +8,7 @@

(defxdoc ubi
:parents (kestrel-utilities history)
:code-source "kestrel/utilities/ubi :DIR :SYSTEM"
Copy link
Member Author

Choose a reason for hiding this comment

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

The first rendering would just make this text into a link with the appropriate link target to github as the doc links currently do. If we want to allow lowercase :dir :system to mean the same thing when rendering, let me know, that would be easy.
Also, eventually I would like to be able to add a copy include-book form to clipboard button to the html xdoc rendering that follows the code-source link.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to use lowercase :dir :system.

Copy link
Member Author

@bendyarm bendyarm Jan 31, 2023

Choose a reason for hiding this comment

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

The proposal now is that if you say

  :code-source "ubi.lisp"

it would be a path relative to the current book, but if you say

  :code-source ("kestrel/utilities/ubi" :dir :system)

it would be relative to the community books.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Would just :code-source "ubi" also work?

Copy link
Member

Choose a reason for hiding this comment

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

Oddly, the email github generated with my comment "Looks good" quoted the old example of the :code-source option! What I think actually looks good is Eric's "The proposal now ...".

Copy link
Member Author

Choose a reason for hiding this comment

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

@ericwhitmansmith Yes, :code-source "ubi" would also work, and because the defxdoc is under acl2/books, the code info would render as

  Code:  (include-book "kestrel/utilities/ubi" :dir :system)

(rendered-symbol-lst parents)
terminal
from
#|| EM: review later
(if (equal from "ACL2 Sources")
Copy link
Member Author

Choose a reason for hiding this comment

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

I took this out for simplicity, since in my test it bloated only about 0.6%
However, once the downstream rendering is working I will put it back in later.

@MattKaufmann
Copy link
Contributor

Just as for the related pull request #1454, I don't want to be involved in this project personally, so please remove me as a reviewer.

Please make sure that :DOC at the terminal or meta-x acl2-doc continue to work with these and subsequent xdoc changes.

@bendyarm bendyarm removed the request for review from MattKaufmann January 26, 2023 06:08
;; :DIR :SYSTEM added)
(cons :from bookname)
;; :code-from is specified by the xdoc writer,
;; and is not currently not modified/normalized
Copy link
Member

Choose a reason for hiding this comment

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

not currently not => currently not

Copy link
Member

Choose a reason for hiding this comment

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

I kind of think we ought to fix this before this enters widespread usage. It would seem strange to me to type in :code-from "my/book/path :DIR :SYSTEM" -- I suggest we allow both :code-from "path" and :code-from ("path" :dir :dirname), normalize both to full-book-names using ACL2 system functions (parse-book-name, or something like it) and then use xdoc's normalize-bookname to get it into the form we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's explore this a little bit. I like allowing ("path" :dir :system) with "path" relative to "acl2/books".

When you write ("path" :dir :dirname) do you really mean ("path" :dir <project-keyword>)?
If so, then I think we should interpret this "path" relative to the the project directory.
It would be confusing if "path" were relative to the current book containing the defxdoc.

Since the :code-source is supplied by the doc developer for doc purposes only, it is good to have clear rules on how the text is processed and what ends up in the rendered doc. I think if you say

  :code-source ("something" :dir :myproject)

then it would be most clear if the "something" is passed unchanged (*) to the sysfile-p (:myproject . "something"), and then rendered in the html doc as

  Code:  (include-book "something" :dir :myproject)

(*) There might be some minor modifications of adding and removing ".lisp", since full-book-names are supposed to have the ".lisp" extensions, but I am mainly referring to the directory of the "something" here.

I don't think that this makes code movement harder. If a doc file is referring to a source file with a path relative to the doc file that ought to remain in the same relative place upon movement, then specify it as the string form, not the list form.

For the string version with a relative path

  :code-source "subdir/file"

the developer story becomes slightly more complex. I think it is reasonable if we want this to be interpreted relative to the current book's path, since it saves the doc developer a lot of typing and makes it easier to move code. If the current book is under a project directory, subdirectory "currdir", then this could still be rendered in the html doc as something like

  Code:  (include-book "currdir/subdir/file" :dir :myproject)

@solswords @ericwhitmansmith What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

This all seems good to me. I think we should make the :code-source as easy as possible to write and maintain (and so support relative paths there). When rendered, the include-book form should, whenever possible, be something that can be used from anywhere (and so should have a :dir option whenever possible).

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good. I could go either way (in the :dir-relative, ("something" :dir :myproject) case) as to whether we do minimal processing and just produce a string directly from that object, or canonicalize using parse-book-name or something similar and then generate the string. I think the main differences would be that the former might allow 1) totally bogus paths, and 2) paths relative to include-book-dirs as well as project dirs, I think 1) is disadvantageous. 2) could be nice except that include-book-dirs aren't global, so potentially a user might have to determine what cert.acl2 file (or whatever) to load to get that include-book-dir. So I guess I'm leaning toward full processing and canonicalization with parse-book-name even for the :dir-relative case.

Copy link
Member

@solswords solswords left a comment

Choose a reason for hiding this comment

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

I guess my code comments here are mostly relative to planned work, not what's done here -- this PR seems pretty harmless in that it just adds the argument to defxdoc/defsection.

;; :DIR :SYSTEM added)
(cons :from bookname)
;; :code-from is specified by the xdoc writer,
;; and is not currently not modified/normalized
Copy link
Member

Choose a reason for hiding this comment

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

I kind of think we ought to fix this before this enters widespread usage. It would seem strange to me to type in :code-from "my/book/path :DIR :SYSTEM" -- I suggest we allow both :code-from "path" and :code-from ("path" :dir :dirname), normalize both to full-book-names using ACL2 system functions (parse-book-name, or something like it) and then use xdoc's normalize-bookname to get it into the form we want.

;; it to a form similar to the :from (for now at
;; least)
; [Pending change]
; (cons :code-from code-source)
Copy link
Member

Choose a reason for hiding this comment

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

why is this still commented out? Shouldn't this change be tested with this uncommented?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because if the xdoc table entry changes now, all the consumers will need to be OK with that, and I was probably missing one when I tried it before since I got errors that were hard to debug. I'm trying for a staged approach to make it easier.

":pkg is not a string that is a known package (or nil)")))
":pkg is not a string that is a known package (or nil)")
(and code-source (not (stringp code-source))
":code-source is not a string (or nil)")))
Copy link
Member

Choose a reason for hiding this comment

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

see my other comment -- I think we should accept ("path" :dir :dirname) as well as just a string here

@acoglio
Copy link
Member

acoglio commented Jul 23, 2023

Should we merge this?

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

5 participants