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

Fixed null safety error in getData() #26

Merged
merged 3 commits into from
Nov 29, 2022

Conversation

timcreatedit
Copy link
Contributor

When attempting to call the getData() method on a nonexistent Reference a CastError is currently thrown due to the method erroneously having a non-nullable type while the original library allows for null to be returned. This PR fixes this problem, so that for example getting the data of a file that wasn't written yet can be unit tested properly.

@atn832
Copy link
Owner

atn832 commented Sep 28, 2022

Looks good. Would you mind also adding a unit test that shows the new behavior? It'll also prevent people from accidentally breaking your change in the future.

@timcreatedit
Copy link
Contributor Author

I've added two tests for the getData() method, one of which failed before the change and passes now :)

@timcreatedit
Copy link
Contributor Author

@atn832 During my testing I also noticed, that the behavior for .child() is inconsistent with the actual CloudFirestore package. It doesn't add a "/" to the path. I also fixed that in the latest commit

@timcreatedit timcreatedit changed the title Fixed null safety error in getData() Fixed null safety error in getData() and .child() behavior Sep 29, 2022
@atn832
Copy link
Owner

atn832 commented Oct 2, 2022

@atn832 During my testing I also noticed, that the behavior for .child() is inconsistent with the actual CloudFirestore package. It doesn't add a "/" to the path. I also fixed that in the latest commit

Thank you for attempting to fix this bug as well. However, we want to keep each PR specific to one issue. Otherwise PRs grow large and they become harder to review. Since they are harder to review, it takes more time, and we may accidentally introduce new bugs. If we stick to one issue, we can merge the PR much faster and more safely.

Also, if some PR causes a regression, it is much easier to revert just that one PR instead of a huge PR that will revert some real fixes and one regression.

I filed #28 so that you can fix it in a separate PR, if you wish to.

For this PR, would you mind reverting the fix to .child()?

Copy link
Owner

@atn832 atn832 left a comment

Choose a reason for hiding this comment

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

The tests look good. One final ask: can you remove the unused import in the tests and the changes to .child()?

test/firebase_storage_mocks_test.dart Outdated Show resolved Hide resolved
@timcreatedit
Copy link
Contributor Author

I've made the changes you requested, sorry it took me so long 😬

@timcreatedit timcreatedit changed the title Fixed null safety error in getData() and .child() behavior Fixed null safety error in getData() Nov 28, 2022
@atn832
Copy link
Owner

atn832 commented Nov 29, 2022

The analyzer job failed with:

info • Only use double quotes for strings containing single quotes • test/firebase_storage_mocks_test.dart:51:50 • prefer_single_quotes

I'll merge it anyway. Please do fix it later when you get the chance. :)

Thank you for the PR.

@atn832 atn832 merged commit 0e0add2 into atn832:master Nov 29, 2022
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.

None yet

2 participants