parse-community / Parse-SDK-Flutter

The Dart/Flutter SDK for Parse Platform
https://parseplatform.org
Apache License 2.0
575 stars 186 forks source link

refactor: Update package_info_plus #934

Closed mbfakourii closed 1 year ago

mbfakourii commented 1 year ago

Pull Request

Issue

Closes: #821

Approach

Update package_info_plus to package_info_plus: ^4.0.2

Tasks

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this pull request!

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 11.11% and no project coverage change.

Comparison is base (a682290) 39.50% compared to head (03ec1fb) 39.50%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #934 +/- ## ======================================= Coverage 39.50% 39.50% ======================================= Files 60 60 Lines 3339 3339 ======================================= Hits 1319 1319 Misses 2020 2020 ``` | [Impacted Files](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) | Coverage Δ | | |---|---|---| | [packages/dart/lib/src/network/dio\_adapter\_io.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvc3JjL25ldHdvcmsvZGlvX2FkYXB0ZXJfaW8uZGFydA==) | `0.00% <0.00%> (ø)` | | | [...ackages/dart/lib/src/network/parse\_dio\_client.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvc3JjL25ldHdvcmsvcGFyc2VfZGlvX2NsaWVudC5kYXJ0) | `0.00% <0.00%> (ø)` | | | [...src/objects/response/parse\_exception\_response.dart](https://app.codecov.io/gh/parse-community/Parse-SDK-Flutter/pull/934?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-cGFja2FnZXMvZGFydC9saWIvc3JjL29iamVjdHMvcmVzcG9uc2UvcGFyc2VfZXhjZXB0aW9uX3Jlc3BvbnNlLmRhcnQ=) | `33.33% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mbfakourii commented 1 year ago
mtrezza commented 1 year ago

Re-running CI to get all tests to pass

mbfakourii commented 1 year ago

It seems that the CI is always failing, and it's not just the coverage upload. Could you take a look?

Yes, Sure

mbfakourii commented 1 year ago

The last error related to the code format is not a problem in my system, but it is a problem in CI!

image

@Nidal-Bakir Can you also check?

mtrezza commented 1 year ago

I think the issue is that the linter is part of the dart framework. So if the Dart beta version enforces a new lint rule, the lint check for that framework version will fail. That's why it's only failing for "Dart beta".

It would be good if we had an output in the log of the lint check, is there an option? I've tried output=show but that just showed all the file contents the linter checks. Alternatively you could use the dart beta version locally to see what the lint error is. Once we know the line rule, we could try to manually set the lint rule in a config file to override it to use the previous behavior. The linter should hopefully support a lint config file.

mbfakourii commented 1 year ago

@mtrezza Why should the dart beta version be checked?

mtrezza commented 1 year ago

It fails dart format, which I assume is because the linter changed in that dart version. We currently don't have the beta version jobs as "required" in the CI, so we could also merge regardless of whether it fails. I guess the ideal strategy would be to find out what format change is causing the issue and probably fix it in the PR.

On the other hand, we could also just remove the lint check from the beta versions, if we consider formatting not be crucial for preparing our code for beta releases. Formatting after all can be easily adapted once the beta version becomes stable.

mtrezza commented 1 year ago

Why removed? Adding beta was debated thoroughly in a previous PR and we deiced to add it. If we now remove it we should find arguments that outweigh the other arguments.

I think we'd need to:

  1. Find out what exactly the linter is complaining about.
  2. If we can't fix it, remove linting for beta, instead of removing beta altogether.
  3. If we can't removing linting for beta, decide whether we should remove beta or just ignore it (it's currently not a required job).
mbfakourii commented 1 year ago

Why removed? Adding beta was debated thoroughly in a previous PR and we deiced to add it. If we now remove it we should find arguments that outweigh the other arguments.

I think we'd need to:

  1. Find out what exactly the linter is complaining about.
  2. If we can't fix it, remove linting for beta, instead of removing beta altogether.
  3. If we can't removing linting for beta, decide whether we should remove beta.

Sorry, I was testing and returned it

mtrezza commented 1 year ago

Do you think you could run dart format locally with the dart beta version to find out what the 1 issue is the linter corrects?

Installing the linux-x64 Dart SDK version 3.1.0-163.1.beta from the beta (release) channel.

mbfakourii commented 1 year ago

Do you think you could run dart format locally with the dart beta version to find out what the 1 issue is the linter corrects?

I will test

mbfakourii commented 1 year ago

@mtrezza It has problems from version 2.18 to 3. I will roll back to the previous version. I think it is better if there is a problem in the beta!

mtrezza commented 1 year ago

Is there no way we can slightly refactor the code so it passes both? For example, change the line length to avoid the line break that seems to cause the issue.

mbfakourii commented 1 year ago

Is there no way we can slightly refactor the code so it passes both?

I will check

mbfakourii commented 1 year ago

fixed

mbfakourii commented 1 year ago

Well done! So we can keep the beat versions in the CI; is this ready for merge?

Yes, it is ready to merge