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

Type links #4256

Merged
merged 1 commit into from
May 18, 2024
Merged

Type links #4256

merged 1 commit into from
May 18, 2024

Conversation

coke
Copy link
Collaborator

@coke coke commented Mar 6, 2023

The problem

Inconsistent formatting when referring to types in the docs that have their own primary page. (#2954)

Solution provided

xt/rakudoc-types.rakutest

@coke coke requested a review from cfa March 6, 2023 02:32
@coke
Copy link
Collaborator Author

coke commented Mar 6, 2023

need to allow #tags on the links, probably.

@cfa
Copy link
Contributor

cfa commented Mar 6, 2023

This is also erroneously treating links like the following as bad,

L<C<Proc/Async|/type/Proc::Async>> should be L<C<Proc/Async>|/type/Proc/Async>

@coke
Copy link
Collaborator Author

coke commented Mar 6, 2023

We should standardize one way or the other, let's pick.

@cfa
Copy link
Contributor

cfa commented Mar 7, 2023

A few more false positive cases:

  • Index entries
    • e.g. Failed test 'X<Scalar> should be L<C<Scalar>|/type/Scalar>'
    • Suggestion: exclude these from consideration altogether.
  • Explicitly formatted terms
    • e.g.
      • 'I<method> should be L<C<method>|/type/method>',
      • 'I<block> should be L<C<block>|/type/block>'
    • Two subissues here:
      1. "method" (like "block", "map", "match", "raku" etc.) should probably be excluded from consideration as FPs are very likely. (More on this below.)
      2. I'm not sure that we should assume that explicitly formatted terms are missing additional formatting codes. It's probably reasonable to omit I<...>, B<...> etc. from consideration altogether (or to have the test check specific codes). This could cover the X<...> case too.
  • Case insensitivity
    • e.g. 'L<exception|/type/Exception> should be L<C<exception>|/type/exception>'
    • Two issues here:
      1. /type/exception was derived from "exception" and located on disk (a byproduct of running this on a case insensitive filesystem?)… but the casing is incorrect (this should be /type/Exception, as it was originally).
      2. When word and type case don't match, perhaps skip? "exception" here—like "method" above—is likely to be a false positive. Nevertheless, requiring consistent case between word and type name might omit a few more FPs?

@coke
Copy link
Collaborator Author

coke commented Mar 7, 2023

  • I only have case insensitive filesystems for easy access testing, agree we have to address that.
  • Will definitely exclude X<>
  • my thought about B<> and I<> was that they were likely intended to be type refs but were non standard, so wanted to call them out. if we want B<stuff>, we could have B<L<C>|..>>?

@cfa
Copy link
Contributor

cfa commented Mar 7, 2023

Sounds good.

(The false positives for B<> and I<> might be addressed by a combination of word exclusion and case sensitivity.)

@@ -10,7 +10,10 @@ Any other formatting code that refers to a type will fail the test; any C<>
that isn't inside of an L<> will fail, and any L<> that doesn't have a C<>
will fail. Links may end with an optional #id.

One exception: Referring to a type on its own page should only use C<>.
Eexceptions:
Copy link
Member

Choose a reason for hiding this comment

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

Ee

@coke
Copy link
Collaborator Author

coke commented May 9, 2023

Rebased accidentally against master and then (hopefully correctly) against main.

@coke coke force-pushed the type-links branch 2 times, most recently from 1cb504e to 04cacea Compare July 30, 2023 19:06
@coke
Copy link
Collaborator Author

coke commented Jul 30, 2023

Rebased accidentally against master and then (hopefully correctly) against main.

Just did the same thing again!

@coke coke force-pushed the type-links branch 2 times, most recently from df79e04 to 2a980a4 Compare May 18, 2024 01:12
Accepted formatting is, e.g.:

L<C<Int>|/type/Int>

Fixup doc/Type/Bool to pass the test.
@coke coke marked this pull request as ready for review May 18, 2024 01:18
@coke
Copy link
Collaborator Author

coke commented May 18, 2024

Most of the issues from cfa's review are covered. One doc file is now passing in the branch - will merge back to main and start addressing the formatting everywhere.

@coke coke merged commit 8f0aeab into main May 18, 2024
@coke coke deleted the type-links branch May 18, 2024 01:19
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.

3 participants