Make proc macro support parameter attributes

[davidpdrsn]

Fixes #421.

I decided to add https://crates.io/crates/proc-macro-error as mentioned here. It made returning a nice error significantly easier.

TODO

• Fix use of old style in other tests/examples. cargo test currently produces quite a few deprecation warnings.
• Make rename = "..." work. @theduke not quite sure how to handle this one. Got a hint?
• Look into allowing docs on arguments with normal attribute Allow argument documentation  #525

[davidpdrsn]

It seems like rustfmt removes the param attributes on CI

Is it running a pre 1.39 version of nightly perhaps? Things are working fine on my machine.

[davidpdrsn]

I see the tests on stable and beta all fail because

---- src/lib.rs - object (line 136) stdout ----
420error[E0658]: attributes on function parameters are unstable
421  --> src/lib.rs:174:9
422   |
42340 | /         #[graphql(
42441 | |             // You can specify default values.
42542 | |             // A default can be any valid expression that yields the right type.
42643 | |             default = true,
42744 | |             description = "Argument description....",
42845 | |         )] arg1: bool,
429   | |__________^

Not quite sure how to resolve that 🤔

[davidpdrsn]

@theduke @LegNeato would one of you be able to skim this and let me know if I'm missing something obvious? If all is good I'll look into updating all the other code using the old style of customising arguments. But just in case something needs to be changed I would like some confirmation before starting that.

[theduke]

Looks good to me!

I'm actually also fine with removing the old format right away without a deprecation period, and publishing it with the async release.

[davidpdrsn]

This should be ready to go now. I have removed support for the old style of customising args and updated all the existing code to use the new style. I was also able to get renaming working.

All thats left is wait for 1.39 to ship.

Let me know if you want me to squash the commits 😊

[davidpdrsn]

I also took the liberty of adding assert-json-diff as a dev dependency. It makes comparing serde_json::Values in tests easier by pin pointing exactly where there are differences. This made implementing the renaming quite a bit easier.

Full disclosure: I maintain assert-json-diff.

[LegNeato]

The formatting seems to have removed some of these?

[davidpdrsn]

@LegNeato I guess those todos were adding in the async-await branch. I created this branch from master.

[davidpdrsn]

Hm so it seems rustfmt on stable (1.4.8-stable) removes parameter attributes. That is why lots of tests failed after I formatted. I guess we can either format on nightly, or wait for a new version of rustfmt to come out that doesn't remove parameter attributes.

I have reverted the formatting commit so at least cargo +nightly test passes.

[LegNeato]

The fix should be on latest stable, right?

[davidpdrsn]

@LegNeato Doesn't seem like it

/U/d/de/n/juniper [parameter-attributes@ce34e54cfdf3c]
$ cargo +nightly fmt

/U/d/de/n/juniper [parameter-attributes@ce34e54cfdf3c]
$ git status
On branch parameter-attributes
Your branch is up to date with 'origin/parameter-attributes'.

nothing to commit, working tree clean

/U/d/de/n/juniper [parameter-attributes@ce34e54cfdf3c]
$ cargo +stable fmt

/U/d/de/n/juniper [parameter-attributes@ce34e54cfdf3c *]
$ git status
On branch parameter-attributes
Your branch is up to date with 'origin/parameter-attributes'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
  modified:   juniper/src/executor_tests/introspection/mod.rs
  modified:   juniper/src/executor_tests/variables.rs
  modified:   juniper/src/macros/tests/args.rs
  modified:   juniper/src/macros/tests/impl_object.rs
  modified:   juniper/src/schema/schema.rs
  modified:   juniper/src/tests/schema.rs

no changes added to commit (use "git add" and/or "git commit -a")

[LegNeato]

How about now? 😁

[davidpdrsn]

@LegNeato I'm a bit behind on some OSS stuff. Hoping to get time this weekend to look into things. Great that async/await support is merged! 🎊

[davidpdrsn]

I see has drifted quite far behind the current master. I'll try to get things up to date so we can finally get it merged.

[booooh]

Checking in a year later -- is this still planned? 😉

[davidpdrsn]

Ah sorry. Dropped the ball on this one.

Unfortunately I don't currently have the bandwidth to drive it home. I'll leave this PR open in case some one wants to pick it up but maintainers would feel free to close it.

[booooh]

Ah sorry. Dropped the ball on this one.

No worries, it seems like most of the content is already in the MR -- so thanks for that!

Unfortunately I don't currently have the bandwidth to drive it home. I'll leave this PR open in case some one wants to pick it up but maintainers would feel free to close it.

I'll try to take a look, and see if I can get it in shape for a review.
I'm guessing the first step would be to rebase this on latest master and then confirm the updated functionality still works as expected?

[davidpdrsn]

I'm guessing the first step would be to rebase this on latest master and then confirm the updated functionality still works as expected?

Yes. Good luck 😊

[booooh]

Hmmm, I started the rebasing effort, but seems like some tests have already been enabled here:

juniper/juniper/src/macros/tests/args.rs

Line 90 in c78045c

fn attr_arg_descr(#[graphql(description = "The arg")] arg: i32) -> i32 {

I realize that this MR also cleans up a lot of the other content, but does this mean the core functionality is available in master - and the docs just need to be updated?

[tyranron]

Done in #971