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

feat: 'auto' option for runPubGetInParallel #731

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tomassasovsky
Copy link

Description

Adds the new value of 'auto' to runPubGetInParallel, which is now default and will only run pub get sequentially if it's being run on a CI instance.

Since the current variable type is of bool for this variable, I had to add some code to handle both the old and new possible data types for parsing. Any alternative ideas for this are more than welcome.

PS: thanks for making such an awesome tool!

Closes #546

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
  • ! -- Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 refactor -- Code refactor
  • ci -- Build configuration change
  • 📝 docs -- Documentation
  • 🗑️ chore -- Chore

Adds the new value of 'auto' to runPubGetInParallel, which is now default and will only run pub get sequentially if it's being run on a CI instance.
Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Missing tests and updated docs, but other than that it looks good!

packages/melos/lib/src/command_configs/bootstrap.dart Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Jun 19, 2024

CLA assistant check
All committers have signed the CLA.

@tomassasovsky
Copy link
Author

Changed the docs to reflect the change, but tests are a bit more complicated cause I don't really understand how things are set up there and how everything interacts with each other just yet. I can take a deeper look into it during the week though.

@@ -183,7 +183,7 @@ dependencyOverridePaths:

Whether to run `pub get` in parallel during bootstrapping.

The default is `true`.
The default is `auto`, which fallbacks to `false` if the machine is a CI instance.
Copy link

Choose a reason for hiding this comment

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

If this change will make into main, I would suggest adding explanation to this line. Why is this the default, what is the issue it resolves, and what could someone who changes this to true expect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good idea

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you look into that @tomassasovsky?

@spydon
Copy link
Collaborator

spydon commented Aug 7, 2024

@tomassasovsky any updates on this? :)

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.

request: dynamic runPubGetInParallel
4 participants