Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullsafety #7

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Nullsafety #7

wants to merge 15 commits into from

Conversation

j4qfrost
Copy link

@j4qfrost j4qfrost commented Mar 1, 2021

Had a thought, assuming that making variables nullable hinders performance, getting rid of the Empty constructor would allow for making fields non-nullable, improving performance. This is purely speculative.

@j4qfrost
Copy link
Author

j4qfrost commented Mar 1, 2021

Depends on this stablekernel/dart-codable#12

@hpoul
Copy link

hpoul commented Mar 10, 2021

fyi, since there hasn't been any reaction for half a year @ #6 i went ahead and just published a _forked version on pub - https://pub.dev/packages/open_api_forked (and https://pub.dev/packages/codable_forked) - i've merged your nnbd changes into it. (https://github.com/hpoul/dart-codable and https://github.com/hpoul/open-api-dart 🤷‍♂️️)

@j4qfrost
Copy link
Author

Tell me how it works for you. There's an internal meeting happening soon, so hopefully we get a definitive answer on what the future of the project looks like. I'll be making efforts to migrate aqueduct to 2.12 on my own regardless.

String? title;
String? description;
String? example;
List<String?>? isRequired = [];
Copy link

Choose a reason for hiding this comment

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

since this is not a boolean, it's a bit weird to change this to isRequired, should probably remain required?

Copy link
Author

Choose a reason for hiding this comment

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

Thought it was weirder since required is now a reserve word.

Copy link
Author

Choose a reason for hiding this comment

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

We could change the name to requirements, but I would rather not keep required.

Copy link

Choose a reason for hiding this comment

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

i've changed it to required in my own fork to keep the existing code working, since I saw no reason to make it more awkward.. but i guess this project is dead anyway 😅️ just thought i might mention it, since it looked like some bulk-fix to me.
fwiw, required is not a reserved word and it seems you can even have a parameter list like void foo({required int required})

Copy link
Author

Choose a reason for hiding this comment

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

Aaa I am mistaken, it's still just a marker but without the @. It still felt weird to me, plus with some linters highlighting the word, it can be distracting to some developers. Either way I think it would still be a better idea to use another word for the variables. We need to discuss who will take ownership of this project in the future and how it fits in with the other aqueduct dependencies.

On a related note, a group of us are going to discuss transitioning aqueduct and the dependencies to an open source community. The conversation is happening right now on Slack, but I would like something more coordinated.

bsutton and others added 2 commits March 31, 2021 21:48
* Applied standard dart formatting.

* Removed deprecated method.

* removed deprecated option.

* upgraded to new version of conduit_codable

* renamed project to conduit_open_api

* Added lint package and resolved all automated fixes.

* manually fixed lints.

* Completed work on migrating to conduit_codable and nnbd migration.

* Released 1.0.0-b1

* removed deprecated authors field.

* removed null test as should never occur.

* changed components so that they return a non-nullable list.

* Remove the curl commands as the unit tests now fetch the required files themselves.

* restored the kubenetes test file as we found a v2 version. reverted some fields to be nullable as the document uses null to know that they were not present in the original document.
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.

3 participants