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

Migrate to null safety #13

Closed
nilsreichardt opened this issue Mar 2, 2021 · 11 comments
Closed

Migrate to null safety #13

nilsreichardt opened this issue Mar 2, 2021 · 11 comments

Comments

@nilsreichardt
Copy link

Description

All dependencies are migrated to null safety. So we should migrate this package to null safety.

Resources

@6h4n3m
Copy link

6h4n3m commented Mar 8, 2021

I do not think the package is maintained anymore.

I've forked and migrated the package myself since I'm using it in one of my projects, you can depend on my fork like this:

liquid_progress_indicator:
    git: https://github.com/6h4n3m/liquid_progress_indicator.git

Some changes will be required to your app/package because some parameters are now required but were not before, but it's as easy as providing some colors/widths...etc.

Edit: To clarify, I'm not planning on making any changes or maintaining the library, you're welcome to fork my own fork and use that in your production code if you're afraid any changes will be introduced into master, or you can even depend on a specific ref.

@JErazo7
Copy link

JErazo7 commented Mar 12, 2021

I do not think the package is maintained anymore.

I've forked and migrated the package myself since I'm using it in one of my projects, you can depend on my fork like this:

liquid_progress_indicator:
    git: https://github.com/6h4n3m/liquid_progress_indicator.git

Some changes will be required to your app/package because some parameters are now required but were not before, but it's as easy as providing some colors/widths...etc.

Edit: To clarify, I'm not planning on making any changes or maintaining the library, you're welcome to fork my own fork and use that in your production code if you're afraid any changes will be introduced into master, or you can even depend on a specific ref.

Can you upload as a new package in pub.dev?

@6h4n3m
Copy link

6h4n3m commented Mar 12, 2021

I do not think the package is maintained anymore.
I've forked and migrated the package myself since I'm using it in one of my projects, you can depend on my fork like this:

liquid_progress_indicator:
    git: https://github.com/6h4n3m/liquid_progress_indicator.git

Some changes will be required to your app/package because some parameters are now required but were not before, but it's as easy as providing some colors/widths...etc.
Edit: To clarify, I'm not planning on making any changes or maintaining the library, you're welcome to fork my own fork and use that in your production code if you're afraid any changes will be introduced into master, or you can even depend on a specific ref.

Can you upload as a new package in pub.dev?

https://pub.dev/packages/liquid_progress_indicator_ns

@slavap
Copy link

slavap commented Apr 22, 2021

@6h4n3m
You have made a mistake in your null safety porting: borderWidth and borderColor are required ONLY if one of them is defined, they could be both null (and it was always working like that). Please fix it. Check in non-null safety version was:

if (borderWidth != null && borderColor == null ||
        borderColor != null && borderWidth == null) {
      throw ArgumentError("borderWidth and borderColor should both be set.");
    }

slavap referenced this issue in 6h4n3m/liquid_progress_indicator May 7, 2021
@volgin
Copy link

volgin commented Jun 1, 2021

@6h4n3m
You have made a mistake in your null safety porting: borderWidth and borderColor are required ONLY if one of them is defined, they could be both null (and it was always working like that). Please fix it. Check in non-null safety version was:

if (borderWidth != null && borderColor == null ||
        borderColor != null && borderWidth == null) {
      throw ArgumentError("borderWidth and borderColor should both be set.");
    }

This code is obviously wrong.

@slavap
Copy link

slavap commented Jun 2, 2021

@volgin What do you mean wrong? It is code BEFORE null safety migration, after migration you have to specify both borderWidth and borderColor - which is wrong of course.

@volgin
Copy link

volgin commented Jun 2, 2021

@slavap Missing parenthesis. It should be:

if ((borderWidth != null && borderColor == null) ||
        (borderColor != null && borderWidth == null)) {
      throw ArgumentError("borderWidth and borderColor should both be set.");
}

This code never did what it was supposed to do.

@slavap
Copy link

slavap commented Jun 3, 2021

@volgin Why? At least in non-migrated to null safety version was no need to define explicitly both borderWidth and borderColor.

@volgin
Copy link

volgin commented Jun 3, 2021

@slavap I understand what you are saying. But the code that you posted does NOT do what you think it does. Without extra parenthesis it makes no sense.

@slavap
Copy link

slavap commented Jun 3, 2021

@volgin What parenthesis are you talking about? This code works and checks that both are null OR both are not null.

@JordanADavies
Copy link
Owner

Done. Sorry it's taken so long.

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

No branches or pull requests

6 participants