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

Add "coqdoc_header" and "coqdoc_footer" fields. #11131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rlepigre
Copy link
Contributor

@rlepigre rlepigre commented Nov 18, 2024

This is an attempt at fixing #11017.

Although I have a working test-case, I have not done much testing so far. I'd appreciate early feedback anyway, if you can find the time to have a quick look @Alizter, @ejgallego.

What remains to be done:

  • Check for language version in the new fields.
  • Update the documentation.
  • Add change log entry.

@rlepigre rlepigre force-pushed the coqdoc-header-footer branch from 2091f8b to 969a669 Compare November 18, 2024 23:45
@maiste maiste added the coq label Nov 19, 2024
@rlepigre rlepigre force-pushed the coqdoc-header-footer branch from 969a669 to afd123d Compare November 19, 2024 19:20
@ejgallego ejgallego self-assigned this Nov 20, 2024
@Alizter
Copy link
Collaborator

Alizter commented Nov 20, 2024

While we are here, shall we add a field_b_o for toc. It should default to enabled, but we can allow it to be set like header and footer.

@Alizter
Copy link
Collaborator

Alizter commented Nov 20, 2024

We should add a test where the header file comes as the target of another rule. We need to check that the dependencies are being correctly handled.

Variable expansion is also another one that should be checked, but I'm not certain what the best way to do that is yet.

@rlepigre
Copy link
Contributor Author

We should add a test where the header file comes as the target of another rule. We need to check that the dependencies are being correctly handled.

Variable expansion is also another one that should be checked, but I'm not certain what the best way to do that is yet.

@Alizter I added a test that should exercise both at once.

While we are here, shall we add a field_b_o for toc. It should default to enabled, but we can allow it to be set like header and footer.

I'm not sure what you have in mind here, actually. Can't we just use the (coqdoc_flags ...) field for that?

@rlepigre rlepigre force-pushed the coqdoc-header-footer branch from 883ea56 to 7b75def Compare November 21, 2024 21:39
@Alizter
Copy link
Collaborator

Alizter commented Nov 21, 2024

While we are here, shall we add a field_b_o for toc. It should default to enabled, but we can allow it to be set like header and footer.

I'm not sure what you have in mind here, actually. Can't we just use the (coqdoc_flags ...) field for that?

we already set --toc by default. I thought it might be a good idea to introduce a field for that as well. It's not a priority however, so you can disregard that comment.

and+ coq_dump = Dune_rules.Coq.Coq_rules.coq_env ~dir >>| Dune_rules.Coq.Coq_flags.dump
and+ coq_dump =
Dune_rules.Coq.Coq_rules.coq_env ~dir
>>| Dune_rules.Coq.Coq_flags.dump ~dir:(Path.build dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I played around a bit with dropping the directory here, and I couldn't find a good way to do it. I'm not too worried about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure what to do there.

doc/changes/11131.md Outdated Show resolved Hide resolved
@Alizter
Copy link
Collaborator

Alizter commented Nov 22, 2024

Does it make sense to pass these flags when in latex mode? I think this might error out coqdoc. Should be simple to add a test for that.

@rlepigre rlepigre force-pushed the coqdoc-header-footer branch from 7b75def to dc1181b Compare November 22, 2024 07:17
@rlepigre
Copy link
Contributor Author

Does it make sense to pass these flags when in latex mode? I think this might error out coqdoc. Should be simple to add a test for that.

The option is actually ignored (I had tested this), but I guess there is no point in introducing potential dependencies for the latex documentation. I pushed a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants