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.

Sign up for GitHub

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

Jump to bottom

👷 Update configuration related files #1091

Merged
merged 11 commits into from
Feb 23, 2024
Merged

👷 Update configuration related files #1091

merged 11 commits into from
Feb 23, 2024

Conversation

AlexV525
Copy link
Contributor

@AlexV525 AlexV525 commented Dec 24, 2023 •
edited
Loading

  • Ignores IDEA related files, VS Code related files, and pubspec_overrides.yaml from the version control.
  • Unify & reduce the complexity in workflows.
  • Updates *.iml.
  • Removes unnecessary platforms from examples.
  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@github-actions github-actions bot added type-infra A repository infrastructure change or enhancement package:cupertino_http Issues related to package:cupertino_http package:cronet_http labels Dec 24, 2023
@AlexV525 AlexV525 marked this pull request as ready for review January 5, 2024 12:52
@AlexV525 AlexV525 changed the title 👷 Update workflows 👷 Update configuration related files Jan 5, 2024
@AlexV525 AlexV525 marked this pull request as draft January 6, 2024 09:09
@AlexV525 AlexV525 mentioned this pull request Jan 10, 2024
Merged
1 task
@AlexV525

This comment was marked as resolved.

Sign in to view
@AlexV525 AlexV525 marked this pull request as ready for review January 12, 2024 04:19
@AlexV525
Copy link
Contributor Author

@brianquinlan

@brianquinlan brianquinlan self-requested a review January 22, 2024 16:18
@brianquinlan
Copy link
Collaborator

Does this change remove the ability to run the example on some platforms?

@AlexV525
Copy link
Contributor Author

Does this change remove the ability to run the example on some platforms?

Yes. cupertino_http's example only has the iOS/macOS platform now.

@AlexV525
Copy link
Contributor Author

@brianquinlan PTAL

@brianquinlan
Copy link
Collaborator

What's your reasoning for making the example only work on iOS/macOS? Just to get rid of extra files?

brianquinlan
brianquinlan reviewed Feb 20, 2024
View reviewed changes
.github/workflows/cupertino.yml
name: Lint and static analysis
runs-on: ubuntu-latest
verify:
name: Format & Analyze & Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change done for efficiency reasons? Do we want the linting to happen with more than one flutter version? Is it possible that causes unsatisfiable lint conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should reduce the job count from 3 to 2. Also more readable that we're checking different Flutter versions compatibility.

pkgs/cronet_http/cronet_http.iml
</content>
<orderEntry type="sourceFolder" forTests="false" />
<orderEntry type="library" name="Dart Packages" level="project" />
<orderEntry type="library" name="Dart SDK" level="project" />
<orderEntry type="library" name="Flutter Plugins" level="project" />
</component>
</module>
</module>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add the newline back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is auto-managed by the IntelliJ IDEA, so when I saved these files (*.iml) their EOF was gone automatically.

pkgs/cupertino_http/cupertino_http.iml
</content>
<orderEntry type="sourceFolder" forTests="false" />
<orderEntry type="library" name="Dart Packages" level="project" />
<orderEntry type="library" name="Dart SDK" level="project" />
<orderEntry type="library" name="Flutter Plugins" level="project" />
</component>
</module>
</module>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add the newline back.

@AlexV525
Copy link
Contributor Author

What's your reasoning for making the example only work on iOS/macOS? Just to get rid of extra files?

Because the plugin only works on iOS/macOS, isn't it?

@brianquinlan
Copy link
Collaborator

What's your reasoning for making the example only work on iOS/macOS? Just to get rid of extra files?

Because the plugin only works on iOS/macOS, isn't it?

Yeah but the example does run on all platforms and I want to be able to test it. Could you restore those files?

@AlexV525
Copy link
Contributor Author

AlexV525 commented Feb 22, 2024 •
edited
Loading

What's your reasoning for making the example only work on iOS/macOS? Just to get rid of extra files?

Because the plugin only works on iOS/macOS, isn't it?

Yeah but the example does run on all platforms and I want to be able to test it. Could you restore those files?

Does it make sense? Running the example on these platforms is meaningless. I can see you may want to avoid the build failure on all platforms when using the plugin, but the plugin only contains iOS/macOS native implementations, so it won't affect any of the other platforms whether you're using it or not.

Maintaining unsupported platforms also increases the complexity of rolling them to a runnable/latest version during the Flutter updates.

@brianquinlan
Copy link
Collaborator

I'm convinced.

@brianquinlan brianquinlan merged commit d5cab74 into dart-lang:master Feb 23, 2024
29 checks passed
@AlexV525 AlexV525 deleted the chore/ci branch February 23, 2024 01:26
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 27, 2024
@devoncarew
[deps] rev http, protobuf, tools, web, yaml_edit
cad61f9 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

http (https://github.com/dart-lang/http/compare/ce0de37..6e0a46f):
  6e0a46f  2024-02-24  Alex Li  [cronet_http] 🏗️ Use dart-define to determine dependency (dart-lang/http#1111)
  d5cab74  2024-02-23  Alex Li  👷 Update configuration related files (dart-lang/http#1091)
  6873731  2024-02-22  Brian Quinlan  Make it possible for `CronetClient` to own the lifetime of an existing `CronetEngine` (dart-lang/http#1137)

protobuf (https://github.com/dart-lang/protobuf/compare/f085bfd..ef0ab7d):
  ef0ab7d  2024-02-23  Sebastian  Optimize repeated field decoding (google/protobuf.dart#911)

tools (https://github.com/dart-lang/tools/compare/9f4e6a4..c7fbf26):
  c7fbf26  2024-02-21  Elias Yishak  Fallback to current datetime when failing on parse session json file (dart-lang/tools#235)

web (https://github.com/dart-lang/web/compare/975e55c..d96c01d):
  d96c01d  2024-02-23  Srujan Gaddam  Use extension types and generics internally (dart-lang/web#184)

yaml_edit (https://github.com/dart-lang/yaml_edit/compare/82ab64d..54884db):
  54884db  2024-02-23  Devon Carew  update to the latest sdk; update lints (dart-lang/yaml_edit#68)

Change-Id: Ib2cb7971df05f4501f81fe5c04b7eaf88d0d93a3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/354381
Commit-Queue: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianquinlan brianquinlan brianquinlan approved these changes

Assignees
No one assigned
Labels
package:cronet_http package:cupertino_http Issues related to package:cupertino_http type-infra A repository infrastructure change or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AlexV525 @brianquinlan