feat: pavex methods error

[Duckaet]

implements issue #537

i have explained the error in detail

[peddermaster2]

A test case would probably be a good idea.

[LukeMathWalker]

Thanks for picking this up!
Let's add a test case to see what the error looks like. You use the other failure tests in pavex_macros as a reference.

[Duckaet]

@peddermaster2 , yes i have updated it, now it only renders the call site

@LukeMathWalker , I have implemented fail test cases

[LukeMathWalker]

@peddermaster2 , yes i have updated it, now it only renders the call site

@LukeMathWalker , I have implemented fail test cases

Looking nice!
It'd be ideal if we could include the call site (i.e. the method attribute) as well as the "header" of the impl block (i.e. everything before the opening brace). It'd make it easier to locate where the error is happening.

[Duckaet]

@LukeMathWalker , yeah that would be nice but we are constrained as the span::join is nightly-only

another workaround is error.combine but that leaves us with two errors and it looks bad

error: `#[pavex::methods]` ....   ::error_handler]`, etc.).
 --> tests/methods/fail/no_pavex_methods.rs:3:1
  |
3 | #[pavex::methods]
  | ^^^^^^^^^^^^^^^^^

error: This .....s unnecessary.
 --> tests/methods/fail/no_pavex_methods.rs:4:1
  |
4 | impl A {
  | ^^^^

what do you suggest?

[LukeMathWalker]

/ok-to-test sha=de1373c

[LukeMathWalker]

@LukeMathWalker , yeah that would be nice but we are constrained as the span::join is nightly-only

another workaround is error.combine but that leaves us with two errors and it looks bad

error: `#[pavex::methods]` ....   ::error_handler]`, etc.).
 --> tests/methods/fail/no_pavex_methods.rs:3:1
  |
3 | #[pavex::methods]
  | ^^^^^^^^^^^^^^^^^

error: This .....s unnecessary.
 --> tests/methods/fail/no_pavex_methods.rs:4:1
  |
4 | impl A {
  | ^^^^

what do you suggest?

I think we may be able to work around the lack of Span::join using syn::Error::new_spanned, but I haven't tried it out.
Anyway, it's not a dealbreaker, I'm happy to merge as is (when AWS recovers from its outage and thus the Internet starts working again).

[Duckaet]

@LukeMathWalker , yeah that would be nice but we are constrained as the span::join is nightly-only
another workaround is error.combine but that leaves us with two errors and it looks bad

error: `#[pavex::methods]` ....   ::error_handler]`, etc.).
 --> tests/methods/fail/no_pavex_methods.rs:3:1
  |
3 | #[pavex::methods]
  | ^^^^^^^^^^^^^^^^^

error: This .....s unnecessary.
 --> tests/methods/fail/no_pavex_methods.rs:4:1
  |
4 | impl A {
  | ^^^^

what do you suggest?

I think we may be able to work around the lack of Span::join using syn::Error::new_spanned, but I haven't tried it out. Anyway, it's not a dealbreaker, I'm happy to merge as is (when AWS recovers from its outage and thus the Internet starts working again).

I think we may be able to work around the lack of Span::join using syn::Error::new_spanned, but I haven't tried it out.
Anyway, it's not a dealbreaker, I'm happy to merge as is (when AWS recovers from its outage and thus the Internet starts working again).

we cant do that reliably ig as syn::Error::new_spanned also uses the same span logic which can't span from call_site() (the attribute) to the impl header(impl A)

the impl header can be inserted in the message block like this =>

error: `#[pavex::methods]` is used to provide context for Pavex attributes on methods (e.g., `#[pavex::get]`, `#[pavex::post]`, `#[pavex::error_handler]`, etc.).
       The `impl A` block contains no methods with Pavex attributes, so `#[pavex::methods]` is unnecessary.
 --> tests/methods/fail/no_pavex_methods.rs:3:1
  |
3 | #[pavex::methods]
  | ^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the attribute macro `pavex::methods` (in Nightly builds, run with -Z macro-backtrace for more info)

should i commit this instead? if it looks good enough

[LukeMathWalker]

should i commit this instead? if it looks good enough

It looks good if the impl header is small enough, but I'm afraid it won't scale very well on larger impls. Let's leave things as they are for now.