Search categories streamlined #159

Altai-man · 2022-04-10T19:00:08Z

This makes creation of references brain dead simple: X<$category,$item> is how it works instead of cryptic rules nobody understands.

Altai-man · 2022-04-10T19:01:26Z

The bad news is I cannot really test it with just Documentable:

➜  Documentable git:(search-categories-streamlined) ./bin/documentable setup -o
Setting up the directory...
Copying files...
Done.
➜  Documentable git:(search-categories-streamlined) ./bin/documentable start -a --force
No pod in cache associated with '/home/koto/Devel/Documentable/doc/HomePage.pod6'. Has the path changed?
  in method pod at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/16A9686EA4CFC0E9418FEB2C3BDDD0FFB09554E7 (Pod::From::Cache) line 118
  in method load at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/CF97824732E11EBB99505A3911A971224519CF0A (Documentable::Registry) line 60
  in method generate-main-page at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/677E0B857640AD7C699D43EAFA5F9550AE91E0B5 (Documentable::DocPage::Factory) line 40
  in method generate-home-page at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/677E0B857640AD7C699D43EAFA5F9550AE91E0B5 (Documentable::DocPage::Factory) line 31
  in sub MAIN at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/5DD7BBCC86020F8BEFA44ABED316A9B23D8F14AC (Documentable::CLI) line 200
  in sub RUN-MAIN at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/5DD7BBCC86020F8BEFA44ABED316A9B23D8F14AC (Documentable::CLI) line 36
  in block <unit> at ./bin/documentable line 3

➜  Documentable git:(search-categories-streamlined)

@JJ any ideas?

JJ · 2022-04-10T19:20:36Z

I'm right now starting my holiday... Won't be able to do much this week, sorry. Will get around to it when I'm back

JJ · 2022-04-21T13:00:03Z

You might need to delete the local cache. It's surprisingly passing (mostly) in CI. I'll have to check what's the deal with circleci

Altai-man · 2022-04-23T12:45:18Z

TODO: document how calculate-category works.

JJ · 2022-04-23T17:27:23Z

The errors in CircleCI seem genuine, including duplicate index terms and so on. Can you please check them out?

JJ · 2022-04-24T11:07:50Z

The bad news is I cannot really test it with just Documentable:

➜  Documentable git:(search-categories-streamlined) ./bin/documentable setup -o
Setting up the directory...
Copying files...
Done.
➜  Documentable git:(search-categories-streamlined) ./bin/documentable start -a --force
No pod in cache associated with '/home/koto/Devel/Documentable/doc/HomePage.pod6'. Has the path changed?
  in method pod at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/16A9686EA4CFC0E9418FEB2C3BDDD0FFB09554E7 (Pod::From::Cache) line 118
  in method load at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/CF97824732E11EBB99505A3911A971224519CF0A (Documentable::Registry) line 60
  in method generate-main-page at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/677E0B857640AD7C699D43EAFA5F9550AE91E0B5 (Documentable::DocPage::Factory) line 40
  in method generate-home-page at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/677E0B857640AD7C699D43EAFA5F9550AE91E0B5 (Documentable::DocPage::Factory) line 31
  in sub MAIN at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/5DD7BBCC86020F8BEFA44ABED316A9B23D8F14AC (Documentable::CLI) line 200
  in sub RUN-MAIN at /home/koto/.rakubrew/versions/moar-2022.03/install/share/perl6/site/sources/5DD7BBCC86020F8BEFA44ABED316A9B23D8F14AC (Documentable::CLI) line 36
  in block <unit> at ./bin/documentable line 3

➜  Documentable git:(search-categories-streamlined)

@JJ any ideas?

The error in CircleCI can give you additional hints, but the error in cache might be solved by deleting the local cache. It sometimes happens when there are changes in the Raku version that generated the cache.

JJ

Besides the local and CI errors that have already been observed, two major problems are the elimination of a test (and corresponding code), not introducing tests for new functionalities, as well as introduction of functionalities not directly related to the stated scope of this PR. I would be very grateful if you considered them carefully.

JJ · 2022-04-24T11:01:19Z

lib/Documentable/Index.pm6

    } else {
-        $name = textify-pod $meta;
+        warn "At $origin.url() $meta.raku() is not formatted properly";


It should probably say what's wrong about the format. Also, is this tested?

Added a note, thanks! Plus a test is added.

JJ · 2022-04-24T11:01:45Z

lib/Documentable/Index.pm6

    }

    nextwith(
        kind     => Kind::Reference,
+        categories => [$category.trim],


Categories are not trimmed in origin?

Not necessary. When creating an Index object, we get raw pod.meta of a X<>, so if the user types X< foo, bar> which is technically correct, both the category and the term will have a space prefix.
Looking back at it, I think the case where the name should be trimmed can be even more common, so I added it to the name too.
And in case the format is wrong, the exception is prevented with .? (the condition has already warned about itself, no need to see another exception).

JJ · 2022-04-24T11:04:54Z

lib/Documentable/Index.pm6

-    my @indices    = $.pod.meta;
-    my $fragment = qq[index-entry{@indices ?? '-' !! ''}{@indices.join('-')}{$index-text ?? '-' !! ''}$index-text]
+    my $indices    = $.pod.meta[0];
+    my $fragment = qq[index-entry{$indices ?? "-$indices[1]" !! ''}{$index-text ?? '-' !! ''}$index-text]


Not entirely clear what is being done here. You're using $indices[1] === $.pod.meta[0][1] which is what?

In the hindsight, this is indeed dubious.

I was taking the first reference of many to create an URL (so take the first reference in a list of X<category,item;category,item...>, then take its 'item' part), which is 1)breaking; 2)wrong.

So I reverted it back to operating on an array and adapted the test to have a lesser change.

However, the important point here is not to inject the category into the URLs, it's the metadata for the reader, not something we want to base the URLs on.

JJ · 2022-04-24T11:06:03Z

lib/Documentable/Primary.pm6

@@ -130,12 +130,12 @@ class Documentable::Primary is Documentable {
            %attr = name       => $name.trim,
                    kind       => Kind::Syntax,
                    subkinds   => @meta || (),
-                    categories => @meta || ();
+                    categories => @(@meta[0]) || ();


OK, before subkinds and categories could be the same. Now, subkinds can be a superset of categories?

JJ · 2022-04-24T11:09:18Z

lib/Documentable/Search.pm6

-                    url      => $doc.url
-                )
-            }).Slip;
+        self.consume-entries-by-kind(@entries, $registry, Kind::Type, 'Types');


Suggestion: DRY to a loop with the types and strings. As a matter of fact, the type can be computed via introspection.

The explicit code like that, just three lines covering three "main" kinds of documents is IMO easier to read, and just a single call is hardly something to be concerned about being duplicated 3 times.
Can it be written in a simpler way using a loop?

JJ · 2022-04-24T11:11:33Z

lib/Documentable/Search.pm6

-                                        category => @docs > 1 ?? $kind.gist !! @docs[0].subkinds[0] || $kind.gist,
-                                        value    => escape($name),
-                                        url      => escape-json("/{$kind.lc}/{good-name($name)}")
+        for Kind::Syntax.Str -> $kind {


Would need to check line by line here, but it looks mostly repeated. And what's not, could probably be factored out to a different function.
Can't see how this is related to categories, except that now category is calculated instead of having a weird formula. That's nice.

You are right, removed the copypaste.

JJ · 2022-04-24T11:19:32Z

lib/Documentable/Search.pm6

    method search-entry(Str :$category, Str :$value, Str :$url is copy) {
+        if %!seen{"$category - $value"}:exists {


OK, this looks for duplicate categories. That's a good thing; however, it's caused tests to fail (maybe), so it's probably something that should be done in a separate PR. I've created #161 but also Raku/doc#1912 is there from some time (you might remember it) so, even if this only creates a warning, I have a hunch it's related to the fails in CircleCI. And at any rate, since it involves two repositories and it's out of scope in this PR, I would really appreciate if you created a different PR with this only so that we can solve it properly before proceeding with search category streamlinization later.

If there is already a test in the doc repo, that's great! I am not sure if the script actually got in the repo though, cannot find it at a glance. But yes, it makes sense to move it there.

JJ · 2022-04-24T11:24:12Z

lib/Documentable/Search.pm6

        $url = $.prefix ?? "/" ~ $.prefix ~ $url !! $url;
-        if ($url ~~ /\.$/) { $url = "{$url}.html" }
-        qq[[\{ category: "{$category}", value: "{$value}", url: "{$url}" \}\n]]
+        qq[[\{ category: "{ $category }", value: "{ $value }", url: "{ $url }" \}\n]]


Why did you eliminate the hack for the pages that end in .?
I would really appreciate if, in big PRs such as this one that contain seemingly unrelated changes, some justification for them would be made. This was introduced in issue #125 to solve issue #72; it's there for a reason. I haven't seen anything to indicate that those URLs are no longer generated. So, again, if you think this is really something that should be tackled differently, open an issue and create a separate PR to discuss possible solutions (including, of course, just eliminating it as worthless technical debt that's no longer relevant)

Originally I was working on the origin of numerous hacks we have - URLs not being properly escaped && depending on the filename to serve pages, so considered it unnecessary, not anymore.
But given the situation I re-instantiated it until we have something working nicely.

JJ · 2022-04-24T11:26:56Z

t/202-references.t

-    for @references -> $ref {
-        is $ref.name.starts-with(" "), False, "leading whitespace";
-    }
+   my $reference = Pod::FormattingCode.new(


Is this changed indentation to 3? Any reason?

Just a mistake, fixed.

JJ · 2022-04-24T11:29:15Z

t/305-search.t

@@ -36,11 +36,6 @@ subtest "search index generation" => {
    }
 }

-subtest "Final dot entry" => {


Related to above. You also eliminate a test. In general, there must be a very good reason to do so. I guess that since URLs are now simply routes in a REST app, you can indeed handle this seamlessly. However, that's not happened yet and in the interim it will simply break several URLs in the static site.

- For Index, just follow the simple "Take first meta as category" or report an error heuristic - For Primary, allow to overload header processor with something custom ($*HEADING-TO-ANCHOR-TRANSFORMER-ACTIONS) - Streamline Search generation: prefer primary pages over secondary ones, detect duplicate search anchors, be better at calculating categories

@JJ

- Revert a number of surplus changes - Tweak logic of URLs for references (@JJ++) - Add more coverage - Make the code a bit more robust against broken syntax references - Factor out, cleanup some code This is a tradeoff between changing numerous things and changing the bare minimum to make things work according to the new spec.

Altai-man · 2022-06-17T16:40:51Z

@JJ had to take some time to get back to this emotional burden attached code, but I believe it's in a better shape now thanks to your comments now.

The tests pass for me locally, I re-instantiated some hacks and overall made the number of changes smaller.

The bad thing is I'm sure we won't be able to get backward compat here (unless explicitly introducing something like $*NEW-REFERENCES-FORMAT kind of flag, which will be both cumbersome and slower), but the older docs revisions can utilize some upper bound fixed version of Documentable, as the features are rarely getting into it now.

Altai-man mentioned this pull request Apr 10, 2022

Change uses of Doc::TypeGraph to the new name as well #158

Closed

Altai-man mentioned this pull request Apr 10, 2022

Search categories update Raku/doc#4051

Merged

JJ requested changes Apr 24, 2022

View reviewed changes

Altai-man added 3 commits June 17, 2022 16:09

Update tests to match new implementation

b3c851f

Altai-man force-pushed the search-categories-streamlined branch from e293340 to 6280206 Compare June 17, 2022 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search categories streamlined #159

Search categories streamlined #159

Altai-man commented Apr 10, 2022

Altai-man commented Apr 10, 2022

JJ commented Apr 10, 2022

JJ commented Apr 21, 2022

Altai-man commented Apr 23, 2022

JJ commented Apr 23, 2022

JJ commented Apr 24, 2022

JJ left a comment

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

JJ Apr 24, 2022

Altai-man Jun 17, 2022

Altai-man commented Jun 17, 2022

		method search-entry(Str :$category, Str :$value, Str :$url is copy) {
		if %!seen{"$category - $value"}:exists {

Search categories streamlined #159

Are you sure you want to change the base?

Search categories streamlined #159

Conversation

Altai-man commented Apr 10, 2022

Altai-man commented Apr 10, 2022

JJ commented Apr 10, 2022

JJ commented Apr 21, 2022

Altai-man commented Apr 23, 2022

JJ commented Apr 23, 2022

JJ commented Apr 24, 2022

JJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Altai-man commented Jun 17, 2022