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

packages upgraded #1947

Closed
wants to merge 11 commits into from
Closed

packages upgraded #1947

wants to merge 11 commits into from

Conversation

hasanm08
Copy link

@hasanm08 hasanm08 commented Mar 24, 2024

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@mishkov
Copy link

mishkov commented Mar 25, 2024

Important PR to me because I cannot use latest version of other packages due to sentry_flutter. So for now I am just gonna depend on this PR

@kuhnroyal
Copy link
Contributor

Sadly this is not how package updates work for well adapted and used packages.

@hasanm08
Copy link
Author

Sadly this is not how package updates work for well adapted and used packages.

how should I upgrade them when no breaking change happened?

@kuhnroyal
Copy link
Contributor

You can locally always run pub upgrade.
For major releases you can override them if there is no breaking change until the sentry package supports the new version.
But sentry supports several versions back and older Dart SDKs so an update should always try to increase the version range and not force the new major version on everyone.
See #1948

@hasanm08
Copy link
Author

hasanm08 commented Mar 27, 2024

You can locally always run pub upgrade. For major releases you can override them if there is no breaking change until the sentry package supports the new version. But sentry supports several versions back and older Dart SDKs so an update should always try to increase the version range and not force the new major version on everyone. See #1948

but it forced package_info_plus version before and I can not use it in my project because I needed package_info_plus latest version to solve the gradle 8.0 issues that is needed in flutter 3.19

@kuhnroyal
Copy link
Contributor

#1948 was merged, you can check how it was solved.
You can locally use something like this:

dependency_overrides:
  package_info_plus: 7.0.0

@hasanm08
Copy link
Author

hasanm08 commented Apr 2, 2024

#1948 was merged, you can check how it was solved. You can locally use something like this:

dependency_overrides:
  package_info_plus: 7.0.0

issue fixed recheck please

@hasanm08

This comment was marked as spam.

@kahest
Copy link
Member

kahest commented Apr 8, 2024

@hasanm08 we merged the required change for package_info_plus in #1948, the release for this is incoming. Thanks for opening this PR and apologies that this went under our radar - in future, please feel free to ping me to get attention on your issues/PRs.
@buenaflor please take a look if the other changes make sense

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.57%. Comparing base (1ac008b) to head (98fcbb0).
Report is 117 commits behind head on main.

Current head 98fcbb0 differs from pull request most recent head ff504ea

Please upload reports for the commit ff504ea to get more accurate results.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #1947    +/-   ##
========================================
  Coverage   88.57%   88.57%            
========================================
  Files         207      213     +6     
  Lines        6897     7110   +213     
========================================
+ Hits         6109     6298   +189     
- Misses        788      812    +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

generally looks okay to me but as far as I can see most of the changes here are applied to examples so they don't really have an impact on the SDK itself.

have you encountered any issues here when running the samples or any specific reason you chose these packages to change the dep ranges? @hasanm08

@hasanm08
Copy link
Author

hasanm08 commented Apr 8, 2024

@hasanm08 we merged the required change for package_info_plus in #1948, the release for this is incoming. Thanks for opening this PR and apologies that this went under our radar - in future, please feel free to ping me to get attention on your issues/PRs.
@buenaflor please take a look if the other changes make sense

My forked branch is uptodate with your branch but still some packages had issue
And still packageInfoPlus needed to be upgraded in sentry_flutter
And this issue distracts my work
Please recheck and merge it

@hasanm08
Copy link
Author

hasanm08 commented Apr 8, 2024

generally looks okay to me but as far as I can see most of the changes here are either dev dependencies, or applied to examples so they don't really have an impact on the SDK itself.

have you encountered any issues here when running the samples or any specific reason you chose these packages to change the dep ranges? @hasanm08

I have issues using sentry_flutter (latest)and wakelock_plus (latest)
Together because of the packages version in the latest package of yours
Nevermind if you don't merge my work, but please fix this issue and publish a new version

@buenaflor
Copy link
Contributor

buenaflor commented Apr 8, 2024

@hasanm08 the only change that you did here that might affect you directly must be the hive pubspec.yaml which adds a lint range is that correct?

I'm not against merging this but I wanna get the context behind it first

I have issues using sentry_flutter (latest)and wakelock_plus (latest)
Together because of the packages version in the latest package of yours

We already know that package_info_plus is causing issues and are releasing a fix for it soon.
Ideally you can provide other errors here you are encountering so we know which dependencies exactly are the problem.

@buenaflor
Copy link
Contributor

just an fyi, the fix for package info is out as of 7.19.0 @hasanm08

@buenaflor buenaflor marked this pull request as draft April 11, 2024 08:29
@buenaflor
Copy link
Contributor

@hasanm08 do you still need these changes?

@hasanm08
Copy link
Author

@hasanm08 do you still need these changes?

Yes please merge it to use latest version

@hasanm08
Copy link
Author

just an fyi, the fix for package info is out as of 7.19.0 @hasanm08

Fixed

@hasanm08
Copy link
Author

@mishkov

@hasanm08 hasanm08 marked this pull request as ready for review April 21, 2024 09:42
@buenaflor
Copy link
Contributor

Ideally you can provide other errors here you are encountering so we know which dependencies exactly are the problem.

Can you provide errors that happen without this PR?

@hasanm08
Copy link
Author

Ideally you can provide other errors here you are encountering so we know which dependencies exactly are the problem.

Can you provide errors that happen without this PR?

package info plus versioning latest is 8.0.0
please merge it to fix

@@ -18,7 +18,7 @@ publish_to: 'none' # Remove this line if you wish to publish to pub.dev
version: 1.0.0+1

environment:
sdk: '2.17.0'
sdk: '>=2.17.1 <4.0.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

The min_version_test project is specifically created to test against the lowest supported SDK version (2.17.0), so this change doesn't seem right. Why is it necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@buenaflor
Copy link
Contributor

package info plus versioning latest is 8.0.0

this is already fixed, I don't think we need this pr anymore

@buenaflor
Copy link
Contributor

Closing this pr as the problems mentioned have already been fixed.

Feel free to ping me if there is anything else

@buenaflor buenaflor closed this Jun 7, 2024
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.

6 participants