-
Notifications
You must be signed in to change notification settings - Fork 13
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
base: master
Are you sure you want to change the base?
Nullsafety #7
Conversation
Depends on this stablekernel/dart-codable#12 |
fyi, since there hasn't been any reaction for half a year @ #6 i went ahead and just published a |
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 = []; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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})
There was a problem hiding this comment.
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.
* 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.
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.