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(mason_cli): mason make --watch #1131

Merged
merged 37 commits into from
Nov 14, 2023

Conversation

alestiago
Copy link
Collaborator

@alestiago alestiago commented Oct 27, 2023

Status

READY FOR REVIEW

Description

Partially resolves #384

Type of Change

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

@alestiago alestiago marked this pull request as draft October 27, 2023 09:49
@alestiago alestiago marked this pull request as ready for review October 30, 2023 09:57
@alestiago
Copy link
Collaborator Author

Drafted, investigating Windows hang.

@alestiago alestiago marked this pull request as draft November 6, 2023 10:43
@alestiago alestiago marked this pull request as ready for review November 7, 2023 12:11
@alestiago
Copy link
Collaborator Author

alestiago commented Nov 13, 2023

This Pull Request is ready to be reviewed. The Windows hang mentioned previously (#1131 (comment)) was fixed (commit f222430).


If you would like to test the change you may:

  1. Clone my mason fork:
git clone https://github.com/alestiago/mason.git
  1. Checkout to the alestiago:alestiago/mason-watch-flag branch.

  2. Install a local version of mason_cli (from packages/mason_cli):

dart pub global activate --source path .
  1. Try it out by making a new local brick and using the --watch flag.

@felangel
Copy link
Owner

felangel commented Nov 13, 2023

I started reviewing yesterday. Will try to wrap up today or tomorrow. One thing to consider is this implementation doesn’t handle the case where I remove a file from the brick as the output will remain polluted with the previous run’s output for that file. Also the default behavior seems weird because you’ll always have conflicts with watch mode so the default prompting behavior doesn’t work well imo 🤷‍♂️

@alestiago
Copy link
Collaborator Author

alestiago commented Nov 13, 2023

I started reviewing yesterday. Will try to wrap up today or tomorrow.

Awesome, thank you for taking a look. Feel free to apply changes as you desire.

One thing to consider is this implementation doesn’t handle the case where I remove a file from the brick as the output will remain polluted with the previous run’s output for that file.

That is true and is a valid observation! I think implementing such case would require taking snapshots of the output directory. We should for example, not delete a file that was already there before the make command. However, I think we could introduce this functionality as another Pull Request.

Also the default behavior seems weird because you’ll always have conflicts with watch mode so the default prompting behavior doesn’t work well imo

Definitely! Personally, every time I use the --watch argument I specify a configuration file to avoid the prompts. As a bonus, by updating the configuration file I get new makes with whatever variables I changed. In addition, I almost always use --on-conflict overwrite when using --watch.

I didn't commit to solving the "prompting" issue since it would be highly opinionated and require an agreement on how to resolve so. Maybe, we could resolve this as another Pull Request? Since it's already solvable by using a configuration file?

Copy link
Owner

@felangel felangel left a comment

Choose a reason for hiding this comment

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

Thanks so much for the contribution! 💙

@felangel felangel changed the title feat(mason_cli): include mason make watch flag feat(mason_cli): mason make --watch Nov 14, 2023
@felangel felangel merged commit 61b956c into felangel:master Nov 14, 2023
7 checks passed
@alestiago alestiago deleted the alestiago/mason-watch-flag branch November 14, 2023 08:29
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.

feat: mason bundle should be able to watch a brick
2 participants