Make arguments required if they are non nullable

[vasilich6107]

fixes #67

Hi) It's my first meaningful PR to this repo.
Please review if the overall flow is ok I'll add more test.

[vasilich6107]

After several iterations of implementation I decided to achieve this incorporating meta package
https://pub.dev/packages/meta

See the relevant discussion here - dart-lang/sdk#4188 (comment)

This will add an analyser safety warnings and will not introduce breaking change as if it was through dart default requiredParameters

[comigor]

Although it doesn't seem so, this is a breking change (because of the need to import meta (which makes sense btw), and the code used with generated ! variables would start make analyzer complain about new @required annotation).

So could you bump it to 3.0.0 (on pubspec.yaml), and add an entry to changelog pointing the change to this PR + warning about meta package?

[comigor]

I'm thinking about extracting this to a function, so you could just short-circuit the fn when a input.isNonNull is found, return true so you could import the directive. Also, importMeta var wouldn't be needed anymore.

[vasilich6107]

While we decided to increment up to version 3.0.0 I would like to discuss an additional improvement. Let's look at the same query as in original issue #67

input PaginationInput {
  limit: Int!
  offset: Int!
}

query SearchDestinationsData($pagination: PaginationInput!) {
    searchDestinations(pagination: $pagination) {
        totalCount
    }
}

The output class for PaginationInput is

class PaginationInput with EquatableMixin {
  PaginationInput();
  ...
}

I can improve my PR to support the next variant:

class PaginationInput with EquatableMixin {
  PaginationInput({ @required this.limit,  @required  this.offset });
  ...
}

[comigor]

you can also use Iterable.any, but it's the same thing

[comigor]

@vasilich6107 Yeah, I agree we should improve this as well as we're bumping a major.

But let's add all parameters to constructors (both nullable and non-nullable), marking the non-nullable ones as @required, ok?

[vasilich6107]

@vasilich6107 Yeah, I agree we should improve this as well as we're bumping a major.

But let's add all parameters to constructors (both nullable and non-nullable), marking the non-nullable ones as @required, ok?

Sure)

[vasilich6107]

I finished with some kind of initial implementation.

Somewhere in tests I replaced one line queries with

var query = r'''
        query some_query($intsNonNullable: [Int]!, $stringNullable: String) { 
            s, 
            i, 
            list(intsNonNullable: $intsNonNullable) 
        }
      ''';

From my perspective this type of notation could be easier to read and edit.

Please review my code.
Tomorrow morning I have a flight to visit FlutterEurope. I'll be back on Monday 27th.

Also I have a question about using a JSON representation for schema instead of simplified graphql notation.

The JSON schema format also hardens the tests implementation. For example in issue #18 user gave us a schema and a sample query. After fixing the bug we could just create an additional test for this case. In case of JSON schema contributor should too much mental efforts on writing a test.

May be we can discuss this topic via call? If it is ok fell free to choose the tool(skype, zoom, etc)

[comigor]

@vasilich6107 I think this PR is good to go, or do you still want to change something?

From my perspective this type of notation could be easier to read and edit.

Yeah, sure!

Also I have a question about using a JSON representation for schema instead of simplified graphql notation. May be we can discuss this topic via call? If it is ok fell free to choose the tool(skype, zoom, etc)

Are you talking about using schema.graphql files instead of .schema.json of introspection as the schema? That's a nice improvement to do (and there's already an issue for this), but no one hasn't started it yet. Feel free to start this if you're interested in! (but of course we can schedule a call if you want)

Also, have a great Flutter Europe event!

[vasilich6107]

Yep, I'm talking about schema.graphql
If this code looks good to you I think we can merge it)

I'll get back to the issues after FlutterEurope.
If there is no special requirements to have schema.json we can omit the call and I'll try to implement the schema.graphql support)

[comigor]

I'm gonna close this PR because I don't have write access to this branch.
Superseeded by #72