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.

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

Test for unresolved references when compiling the manual via makedoc.g #84

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Test for unresolved references when compiling the manual via makedoc.g #84

merged 1 commit into from
Feb 23, 2022

Conversation

wilfwilson
Copy link
Member

This is a proposal; I saw that @james-d-mitchell had added such a think to the Semigroups package's .release script, and I thought it was a good idea.

As a first step before investing any further work on this: do you think it's a good idea as a general feature? Could you imagine wanting to include this, possibly only enabled by an option?

@fingolfin
Copy link
Member

Sounds useful!

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented May 19, 2021 •
edited
Loading

Possibly works, since a package manual compiles only once, unlike GAP manuals which are compiled twice to get cross-refs right.

What about packages which have more than one manual book though? E.g. GAPDoc or https://github.com/gap-packages/sonata - although it's not a suitable example, it uses gapmacro.tex so maybe the test will pass always and will fail to notice unresolved references (and GAPDoc is not using ReleaseTools).

@wilfwilson
Copy link
Member Author

wilfwilson commented May 19, 2021 •
edited
Loading

This proposal would so far only apply to packages whose documentation can be compiled via the makedoc.g file. Packages that use doc/make_doc (like Sonata) would need a separate check.

The main possible problem that I see with this change is that there may well be packages that currently do have unresolved references, (or people doing releases who don't necessarily have the right manuals compiled) but who don't care about that fact. This change would add a stumbling block for them.

fingolfin
fingolfin reviewed May 20, 2021
View reviewed changes
release-gap-package
if not IsPackageMarkedForLoading("$PKG", "") then
SetPackagePath("$PKG", ".");
fi;
Read("makedoc.g");
GAPInput
! grep -E "WARNING: non resolved reference" makedoc.log
rm -f makedoc.log
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also be done for the second run of makedoc.g? After all, they happen in different directories, and so it could happen that in one run the references can be resolved, but in the other they can't.

@wilfwilson
Copy link
Member Author

I didn't realise until now, this topic was already raised in issue #40. I'll think further about the best way of handling this.

@wilfwilson wilfwilson mentioned this pull request May 21, 2021
Merged
@wilfwilson wilfwilson force-pushed the doc-test-for-unresolved-references branch from 7f44c98 to 496e9c5 Compare June 9, 2021 11:10
@wilfwilson wilfwilson force-pushed the doc-test-for-unresolved-references branch from 496e9c5 to 94e8835 Compare January 25, 2022 17:18
@wilfwilson wilfwilson changed the title Test for unresolved references when compiling the manual Test for unresolved references when compiling the manual via makedoc.g Jan 25, 2022
@wilfwilson
Copy link
Member Author

wilfwilson commented Jan 25, 2022 •
edited
Loading

I will just limit this PR to dealing with the case that the manual is compiled via makedoc.g.

I want to add something to this PR before I consider it to be finished:

  • Give some helpful output when the check fails

@wilfwilson wilfwilson force-pushed the doc-test-for-unresolved-references branch from 94e8835 to ad4e02b Compare January 25, 2022 17:35
@wilfwilson
Copy link
Member Author

I think this should be alright now, but seeing as there is no automated testing for this repository, I want to test the script manually with this PR before the PR gets merged.

@fingolfin
Copy link
Member

Thank you for working on this.

Yeah, lack of testing here is really bad. But I can't find the time and energy to set that up, sorry (perhaps one distant day during a GAP Day coding sprint... but soooo many other things seem more important... sigh).

fingolfin
fingolfin approved these changes Jan 25, 2022
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Feel free to merge it yourself once you are happy with your own tests

@wilfwilson
Copy link
Member Author

Thanks. I'm happy now.

@wilfwilson
Copy link
Member Author

@fingolfin I don't have the permissions to merge this, so please could you do it.

@fingolfin fingolfin merged commit e556a8f into gap-system:master Feb 23, 2022
@wilfwilson wilfwilson deleted the doc-test-for-unresolved-references branch February 23, 2022 21:49
@fingolfin fingolfin mentioned this pull request Nov 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin 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
@wilfwilson @fingolfin @olexandr-konovalov