Adding GQL prefix on important components #78

IcanDivideBy0 · 2020-05-11T16:53:56Z

This what I have so far
I can't find the button to mark the PR as Draft, is that something that should be enabled at project level ?

what's missing:

nicolaiunrein · 2020-05-11T18:42:18Z

I wonder if it's more idiomatic to name the types eg. GqlValue instead of GQLValue like in TcpStream of PartialEq but I think because of the lowercase "L" it gets difficult to read.

phated · 2020-05-11T18:43:56Z

+1 to @nicolaiunrein - I like the lowercase Gql prefix

IcanDivideBy0 · 2020-05-11T22:22:41Z

I might have under estimated the task 😂 but it's finally done!

IcanDivideBy0 · 2020-05-11T22:30:48Z

src/prelude.rs

+// types
+pub use crate::{
+    GqlContext, GqlData, GqlError, GqlFieldResult, GqlID, GqlInputValueResult, GqlQueryBuilder,
+    GqlResult, GqlSchema, GqlValue, GqlVariables,
+};
+
+// traits
+pub use crate::{ErrorExtensions, IntoGqlQueryBuilder, ResultExt, Type};
+
+// procedural macros
+pub use crate::{
+    GqlEnum, GqlInputObject, GqlInterface, GqlObject, GqlScalar, GqlSimpleObject, GqlSubscription,
+    GqlUnion,
+};


Here's the prelude content

phated

Impressive amount of changes, awesome job! I just did a quick scroll-through of the code and had a couple questions.

phated · 2020-05-11T22:25:29Z

async-graphql-actix-web/src/lib.rs

-pub struct GQLRequest(QueryBuilder);
+/// It's a wrapper of `GqlQueryBuilder`, you can use `GQLRequest::into_inner` unwrap it to `GqlQueryBuilder`.
+/// `async_graphql::IntoGqlQueryBuilderOpts` allows to configure extraction process.
+pub struct GQLRequest(GqlQueryBuilder);


Should GQLRequest be updated too?

yes I think we should change GQLResponse and GQLRequest.
There also another GQLError in http module, but it's just a wrapper around &GqlError to implement serialize, I guess we can implement Serialize on GqlError directly now

phated · 2020-05-11T22:29:18Z

src/lib.rs

+pub use async_graphql_derive::GqlScalar;
+
+mod deprecated {
+    pub use super::GqlContext as Context;


Is there a way to annotate deprecations in rust?

I'll check that tomorrow :)

IcanDivideBy0 · 2020-05-11T22:32:34Z

src/lib.rs

+mod deprecated {
+    pub use super::GqlContext as Context;
+    pub use super::GqlContextBase as ContextBase;
+    pub use super::GqlData as Data;
+    pub use super::GqlDataSource as DataSource;
+    pub use super::GqlEnum as Enum;
+    pub use super::GqlError as Error;
+    pub use super::GqlFieldResult as FieldResult;
+    pub use super::GqlID as ID;
+    pub use super::GqlInputObject as InputObject;
+    pub use super::GqlInputValueResult as InputValueResult;
+    pub use super::GqlInterface as Interface;
+    pub use super::GqlObject as Object;
+    pub use super::GqlQueryBuilder as QueryBuilder;
+    pub use super::GqlResult as Result;
+    pub use super::GqlScalar as Scalar;
+    pub use super::GqlSchema as Schema;
+    pub use super::GqlSimpleObject as SimpleObject;
+    pub use super::GqlSubscription as Subscription;
+    pub use super::GqlUnion as Union;
+    pub use super::GqlValue as Value;
+    pub use super::GqlVariables as Variables;
+}
+
+#[doc(hidden)]
+pub use deprecated::*;


Previous exports names are still available here, the whole project build and pass tests with this module disabled

sunli829 · 2020-05-12T01:07:30Z

The button convert to draft is below the reviews on the right.

sunli829 · 2020-05-12T02:08:08Z

When I woke up and saw this huge change, I was very excited and thank @IcanDivideBy0 for his efforts.

But I also carefully considered the necessity of this Gql prefix.
Maybe it can be replaced with use async_graphql as gql, which should be the idiom of rust?

When they need to use prelude, I think in most cases it is use async_graphql::prelude::*, and then the renaming is meaningful.

So I hope we can discuss the necessity of this PR again.

I'm so sorry, this is my mistake, I did not think about it yesterday, and here I solemnly apologize to @IcanDivideBy0. 🙇

vkill · 2020-05-12T02:08:41Z

Oh god, don't add prefix, we should use module.

phated · 2020-05-12T02:18:53Z

Currently, ID in the prelude would collide with my ID in code because I need the newtype pattern to work with Diesel. These are the type of errors we are trying to avoid.

vkill · 2020-05-12T04:30:48Z

@phated Don't use use async_graphql::*;, you can use use async_graphql::ID as GQLID;

phated · 2020-05-12T05:09:37Z

@vkill you are missing the point of a prelude. We are expected to import *

sunli829 · 2020-05-12T05:13:27Z

Renaming only the prelude export is my current thinking. 😁

crate::prelude::ID to crate::prelude::GqlID

nicolaiunrein · 2020-05-12T05:17:33Z

I feel really sorry too 😢. So if the prefix is not an option what about only exporting the (convenience-)traits in the prelude with the *Prelude suffix as suggested here? and establish as a convention to structure imports like this:

use async_graphql::{ self as gql, prelude::* };

#[gql::SimpleObject]
struct MyObject {
    id: gql::ID,
    any: gql::Any,
    // ...
}

That would mean we need to re-export missing frequently used types at the crate root level. Which we are doing already.

sunli829 · 2020-05-12T05:26:39Z

How about this?

use async_graphql::prelude::*;

#[GqlSimpleObject]
struct MyObject {
    id: GqlID, // GQL_ID, GID
    any: GqlAny,
    // ...
}

nicolaiunrein · 2020-05-12T05:29:51Z

Besides the fact that it looks like Java 😆 I really don't think having two names point to the same type is a good idea.

phated · 2020-05-12T05:52:16Z

We shouldn't have 2 names, but we also cannot export very generic names from the prelude. How do you want to do both without these changes?

nicolaiunrein · 2020-05-12T05:58:55Z

We shouldn't have 2 names, but we also cannot export very generic names from the prelude. How do you want to do both without these changes?

The only solution then is not to include the types in the prelude. And only have traits there which are already generically implemented but required to be in the namespace.

I think renaming certain types like Enum to GqlEnum or even GraphqlEnum or GraphQLEnum may be justified because it really is a "GraphQL Enum" as defined in the spec after all. I am more concerned with types like Result, Data, Error etc.

phated · 2020-05-12T06:01:53Z

I encountered the problem with Result last night while trying to do some upgrades. This is why I support the prefixed names.

IcanDivideBy0 · 2020-05-12T06:47:13Z

@sunli829

I'm so sorry, this is my mistake, I did not think about it yesterday, and here I solemnly apologize to @IcanDivideBy0.

It's all fine really, I should have thought of it too :)

I like the use async_graphql as gql version, I think if we put it everywhere in tests and documentation, most people will use it this way, hence the need for a prelude will disolve.

// I don't have strong opinion on this:
use async_graphql::prelude::*;
use async_graphql as gql;

// but...
#[GqlObject]
#[gql::Object] // I find this one more readable

That being said I think we should keep some of the prefixed version: GqlResult GqlError GqlValue. As you all know, those three are really common names in rust world, and while refactoring the code I found many place that required namespace juggling due name clashes around these three particular types.

IcanDivideBy0 · 2020-05-12T06:58:06Z

use async_graphql as gql; can even be completely removed if users declares the dependency as follows:

gql = { package = "async_graphql", version = "..." }

I'm using nalgebra-glm extensively on other project and this is the more convenient way I found to do so, renaming the package to glm in cargo.toml

Adding a note on this in the Book/readme might be of some interrest

sunli829 · 2020-05-12T06:59:00Z

That being said I think we should keep some of the prefixed version: GqlResult GqlError GqlValue. As you all know, those three are really common names in rust world, and while refactoring the code I found many place that required namespace juggling due name clashes around these three particular types.

I'm really happy to see you are not frustrated by this. 🍺
I support the naming of GqlResult and GqlError, but I especially don't like GqlFieldResult. I cried when I saw this in the morning.

nicolaiunrein · 2020-05-12T07:01:38Z

@IcanDivideBy0 I agree mostly on what you are saying. Two points though:

Leaving the the prefix on eg. GqlError would result in using gql::GqlError. I don‘t see the point here.
The prelude might still be neccessary for genericly implemented traits. Although I don‘t know how many there are.

sunli829 · 2020-05-12T07:03:01Z

But I remember I read in the rust book that IoError is not recommended, but io::Error should be used instead.

IcanDivideBy0 · 2020-05-12T07:03:49Z

@nicolaiunrein

GqlError would result in using gql::GqlError

You're right, that's ugly too :/

sunli829 · 2020-05-12T07:05:05Z

Like serde::Result, actix_web::Result, they are not bothered by the problem of naming Result.

nicolaiunrein · 2020-05-12T07:05:09Z

But I remember I read in the rust book that IoError is not recommended, but io::Error should be used instead.

That‘s what I‘m saying.

IcanDivideBy0 · 2020-05-12T07:05:12Z

Damn, naming things is HARD 😂

sunli829 · 2020-05-12T07:08:00Z

I also think prelude is necessary. In fact, the most important thing is to import the necessary traits.

sunli829 · 2020-05-12T07:09:25Z

In fact, unless writing test code, I never import *

sunli829 · 2020-05-12T07:16:48Z

The most fundamental reason is that the name gql is occupied by others. Maybe we can discuss with him to surrender the name? 😂

IcanDivideBy0 · 2020-05-12T07:17:43Z

Up until now Guard and DataSource are the two only traits I've had to import in my code base, and those two are not really general purpose

sunli829 · 2020-05-12T07:22:07Z

I just thought ErrorExtensions, ResultExt will be more commonly used.

IcanDivideBy0 · 2020-05-12T07:51:06Z

Well I just tried to rename to gql in Cargo.toml and this breaks all macros:

   |
18 | #[gql::Object]
   | ^^^^^^^^^^^^^^ use of undeclared type or module `async_graphql`
   |

but it works fine without renaming in cargo.toml and using use async_graphql as gql;

Edit:
this can possibly be fixed by using https://docs.rs/proc-macro-crate/0.1.4/proc_macro_crate/index.html

nicolaiunrein · 2020-05-12T08:02:49Z

I just thought ErrorExtensions, ResultExt will be more commonly used.

If that's the only two traits then I think it's not worth a prelude. I don't know how many people are actually using those. In Apollo there is a section in the book but I think there is trend in GraphQL towards returning result-enums much like in Rust to get typed errors. In fact that's what I started doing after implementing ErrorExtensions 😆

sunli829 · 2020-05-12T08:35:26Z

Let's put this issue aside for now. 😁

Adding GQL prefix on important components

92f4058

IcanDivideBy0 mentioned this pull request May 11, 2020

Initial add prelude module. #74

Closed

IcanDivideBy0 commented May 11, 2020

View reviewed changes

phated reviewed May 11, 2020

View reviewed changes

IcanDivideBy0 commented May 11, 2020

View reviewed changes

sunli829 marked this pull request as draft May 12, 2020 01:08

sunli829 closed this May 13, 2020

Adding GQL prefix on important components #78

Adding GQL prefix on important components #78

Conversation

IcanDivideBy0 commented May 11, 2020 • edited

nicolaiunrein commented May 11, 2020

phated commented May 11, 2020

IcanDivideBy0 commented May 11, 2020

IcanDivideBy0 May 11, 2020

Choose a reason for hiding this comment

phated left a comment

Choose a reason for hiding this comment

phated May 11, 2020

Choose a reason for hiding this comment

IcanDivideBy0 May 11, 2020 • edited

Choose a reason for hiding this comment

phated May 11, 2020

Choose a reason for hiding this comment

IcanDivideBy0 May 11, 2020

Choose a reason for hiding this comment

IcanDivideBy0 May 11, 2020

Choose a reason for hiding this comment

sunli829 commented May 12, 2020 • edited

sunli829 commented May 12, 2020

vkill commented May 12, 2020

phated commented May 12, 2020

vkill commented May 12, 2020

phated commented May 12, 2020

sunli829 commented May 12, 2020

nicolaiunrein commented May 12, 2020 • edited

sunli829 commented May 12, 2020

nicolaiunrein commented May 12, 2020

phated commented May 12, 2020

nicolaiunrein commented May 12, 2020

phated commented May 12, 2020

IcanDivideBy0 commented May 12, 2020 • edited

IcanDivideBy0 commented May 12, 2020 • edited

sunli829 commented May 12, 2020

nicolaiunrein commented May 12, 2020

sunli829 commented May 12, 2020

IcanDivideBy0 commented May 12, 2020 • edited

sunli829 commented May 12, 2020

nicolaiunrein commented May 12, 2020

IcanDivideBy0 commented May 12, 2020

sunli829 commented May 12, 2020

sunli829 commented May 12, 2020

sunli829 commented May 12, 2020 • edited

IcanDivideBy0 commented May 12, 2020

sunli829 commented May 12, 2020 • edited

IcanDivideBy0 commented May 12, 2020 • edited

nicolaiunrein commented May 12, 2020

sunli829 commented May 12, 2020 • edited

IcanDivideBy0 commented May 11, 2020 •

edited

IcanDivideBy0 May 11, 2020 •

edited

sunli829 commented May 12, 2020 •

edited

nicolaiunrein commented May 12, 2020 •

edited

IcanDivideBy0 commented May 12, 2020 •

edited

IcanDivideBy0 commented May 12, 2020 •

edited

IcanDivideBy0 commented May 12, 2020 •

edited

sunli829 commented May 12, 2020 •

edited

sunli829 commented May 12, 2020 •

edited

IcanDivideBy0 commented May 12, 2020 •

edited

sunli829 commented May 12, 2020 •

edited