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

update allowReuse nullability #8

Merged
merged 1 commit into from
Aug 22, 2021
Merged

Conversation

hpoul
Copy link
Contributor

@hpoul hpoul commented Aug 2, 2021

imho allowReuse should either be a non-nullable value or the default value should be used as a fallback when accessed.

otherwise callers can pass in allowReuse: null and cause a crash. 😳

@isoos
Copy link
Owner

isoos commented Aug 2, 2021

@hpoul: good catch, thank you! I am wondering, and maybe you have an opinion on that: is it good that the two method has a different default value? And if we change them, what would be the good default?

Also: could you please make a note about this in the CHANGELOG and bump the version in pubspec.yaml?

@hpoul hpoul force-pushed the allow-reuse-nullability branch from 61264cc to 1c630e2 Compare August 22, 2021 13:06
@hpoul
Copy link
Contributor Author

hpoul commented Aug 22, 2021

rebased, updated CHANGELOG and pubspec.yaml.

I don't think it's a good idea to have different defaults, especially if it's not documented why there is a difference. But it's probably a worse idea to change it 😅️

@isoos isoos merged commit 7763f98 into isoos:master Aug 22, 2021
@isoos
Copy link
Owner

isoos commented Aug 22, 2021

Thanks! Should I release this, or do you have something else in the works?

@hpoul
Copy link
Contributor Author

hpoul commented Aug 22, 2021

@isoos thx, that's it, except you can give me hints on how to fix #11 :) i don't understand why there is a difference between query and execute in the first place 🤔

@isoos
Copy link
Owner

isoos commented Aug 22, 2021

Unfortunately I have no idea on how that code was created and why it differs, I become involved with the package much later.

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.

2 participants