Skip to content

Create CroRouter still with error#2

Open
FCO wants to merge 1 commit into
mainfrom
CroRouter
Open

Create CroRouter still with error#2
FCO wants to merge 1 commit into
mainfrom
CroRouter

Conversation

@FCO

@FCO FCO commented Feb 24, 2024

Copy link
Copy Markdown
Owner

No description provided.

@FCO
FCO force-pushed the main branch 2 times, most recently from a10cbad to 71d6156 Compare February 28, 2024 01:36

@hermes-fco hermes-fco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review \u2014 \n\nFound 2 bugs and some style suggestions.\n\n---\n\n### \ud83d\udc1b Bug #1: crashes on undefined body\n\n****\n\n\n is optional (), defaults to . When no request body is sent (GET requests, or POST with no body), calling on throws. Fix:\n\n\n\n---\n\n### \ud83d\udc1b Bug #2: missing \n\n****\n\n skips setting . Without this dynamic variable, the wrapped methods from the trait (line 93-105) won't return the endpoint path \u2014 they'll call and execute the original method body instead. This means endpoints on class methods () won't generate correct htmx paths when rendering.\n\nAlso, extracts at line 65, but at line 79 assigns directly. These should behave consistently.\n\nFix :\n\n\n---\n\n### \ud83d\udca1 Style: shadows built-in \n\n****\n\n\nNaming an attribute shadows the built-in introspection method. Consider renaming to or .\n\n---\n\n### \ud83d\udca1 Style: fat arrows in \n\n****\n\nCan be simplified with colon pair:\n\nOr even:\n\n\n---\n\n### \u2705 What's Good\n\n- The + Cro route integration is clean and well-structured\n- as a SetHash-based registry with method chaining is elegant\n- The trait with method wrapping for htmx path generation is clever\n- example is complete and runnable\n- tag for rendering sub-components is a smart pattern\n- Already using colon pairs in \ud83d\udc4f\n\n---\n\nReviewed by Hermes Agent

@hermes-fco hermes-fco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review — Create CroRouter

Found 2 bugs and some style suggestions.

🐛 Bug #1: $data.pairs crashes on undefined body

lib/HTML/Component/CroRouter.rakumod:6

my %data is Map = $data.pairs;

$data is optional ($data?), defaults to Any. When no request body is sent (GET requests, or POST with empty body), calling .pairs on Any throws. Fix:

my %data is Map = $data.defined ?? $data.pairs !! ();

🐛 Bug #2: run-undefined missing $*HTML-COMPONENT-RENDERING

lib/HTML/Component/Endpoint.rakumod:72-84

run-undefined doesn't set $*HTML-COMPONENT-RENDERING = True. Without this, the wrapped methods from the %endpoint trait won't return the endpoint path — they call nextsame and execute the original method body. This means endpoints on class methods won't generate correct htmx paths during rendering.

Also, run-defined extracts $snippet.HTML at line 65, but run-undefined at line 79 assigns $component.RENDER: $snippet directly — inconsistent return handling.

Fix:

method run-undefined(|data) {
  my $component = $!class;
  my Capture $cap .= new: :hash(%(|$!capture.hash, |data.hash)), :list[|$!capture.list, |data.list];
  my $method-output = $!class."{&!method.name}"(|$cap);
  if $!return-component {
    require ::("HTML::Component::Tag::SNIPPET");
    my $snippet = ::("HTML::Component::Tag::SNIPPET").new;
    my $*HTML-COMPONENT-RENDERING = True;
    $component.RENDER: $snippet;
    $method-output = $snippet.HTML;
  }
  my $ret = &!return.(:$component, :$method-output);
  return $ret.HTML if $ret.^can: "HTML";
  $ret
}

💡 Style: &!method shadows built-in

lib/HTML/Component/Endpoint.rakumod:5

has &.method is required;

Naming an attribute .method shadows the built-in .method introspection. Consider renaming.

💡 Style: EndpointList.get fat arrow

lib/HTML/Component/EndpointList.rakumod:16

%pars.kv.map: -> $key, $value { $endpoint.matches: |%($key => $value) }

Could use colon pair: :{ :$key($value) }

✅ What's Good

  • root-component + Cro route integration is clean
  • EndpointList SetHash registry with .get chaining — elegant
  • %endpoint trait with method wrapping for htmx — clever
  • cro-todo.raku example complete and runnable
  • SNIPPET tag for sub-components — smart pattern
  • Already using colon pairs in &!return.(:$component, :$method-output) 👏

Reviewed by Hermes Agent

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.

2 participants