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

withBuiltChild fix #1029

Closed
wants to merge 5 commits into from
Closed

Conversation

subzero911
Copy link
Contributor

@subzero911 subzero911 commented Dec 5, 2024

I fixed Observer.withBuiltChild constructor.
I removed runtime asserts and unneeded nullables. So now it's neat.

I also added an example to the docs.


Pull Request Checklist

  • If the changes are being made to code, ensure the version in pubspec.yaml is updated.
  • Increment the major/minor/patch/patch-count, depending on the complexity of change
  • Add the necessary unit tests to ensure the coverage does not drop
  • Update the Changelog to include all changes made in this PR, organized by version
  • Run the melos run set_version command from the root directory
  • Include the necessary reviewers for the PR
  • Update the docs if there are any API changes or additions to functionality

@subzero911
Copy link
Contributor Author

@pavanpodila @amondnet

@subzero911
Copy link
Contributor Author

subzero911 commented Dec 16, 2024

image

@pavanpodila
CI/CD says don't use explicit types for constructor super parameters. But I see nothing bad with it. You'd better set less strict linter rules.

@pavanpodila
Copy link
Member

image

@pavanpodila

CI/CD says don't use explicit types for constructor super parameters. But I see nothing bad with it. You'd better set less strict linter rules.

I'm using default rules with no overrides. These are the ones recommended by Flutter itself

@pavanpodila
Copy link
Member

We should also upgrade the lints packages

@subzero911
Copy link
Contributor Author

image

It isn't shown as an error in the IDE, but CI/CD fails on this line for some reason.

@pavanpodila
Copy link
Member

let's remove the type and try ?

@subzero911
Copy link
Contributor Author

Anyway, it's out of scope of the current MR. It should be done in a separate MR.

@subzero911 subzero911 mentioned this pull request Dec 16, 2024
7 tasks
@subzero911
Copy link
Contributor Author

@pavanpodila are you crazy?
Why did you close this PR?!
I waited for 2 weeks to merge it!!!

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