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

Add -w shorthand for --watch CLI flag #1276

Merged
merged 2 commits into from Apr 15, 2021
Merged

Add -w shorthand for --watch CLI flag #1276

merged 2 commits into from Apr 15, 2021

Conversation

henrycatalinismith
Copy link
Contributor

Hey there! I noticed the help-wanted label on #1145 and thought it might be a nice way to learn how to contribute to one of my fav open source projects, so I've had a try at it. Hopefully I've done a decent job here but no stress if not and you need to reject or point me at some extra steps, or if you're just a little too busy for this kind of thing right now! πŸ˜‡

The code change here was really tiny. I looked into adding some tests in test/cli/shared/watch.dart but it looks like the shorthand functionality is coming from ArgParser so I wasn't sure how much of the existing --watch tests you'd wanna duplicate for verifying that. I've signed the CLA and checked the branch with pub run grinder, dartanalyzer lib test and pub run test -x node so I think I covered those bases anyway. Here's how it looks in the example output when I do dart2native -o bin/sass bin/sass.dart && ./bin/sass. Seems to come through automatically, which is nice!

━━━ Other ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
-w, --watch                    Watch stylesheets and recompile when they change.
    --[no-]poll                Manually check for changes rather than using a native watcher.
                               Only valid with --watch.

I've done some manual testing locally too and it's working great. Oh and I have a sass-site commit ready documenting the new shorthand in case you're keen to have that right away. It occurred to me though that you might also wanna delay shipping that for a moment or two to give the new flag a chance to propagate among the install base first.

Let me know if there's a specific bit of automated testing you'd like me to add, or if I've just completely missed the mark here! It's been fun getting this far and I'm happy to do a bit of extra work on this PR or close it unmerged if necessary!

@henrycatalinismith
Copy link
Contributor Author

Oh interesting, one of the test failures looked like it might have been caused by my change here when I spotted the filename, so I was worried for a second.

00:31 +0 -6: [VM] test\cli\node\watch_test.dart: (setUpAll) [E]
182
  IsolateSpawnException: Unable to spawn isolate: Error: Cannot run with sound null safety, because the following dependencies
183
  don't support null safety:

I looked into some of the other PRs in the queue though and found the same error on the "Fix the name of the blackness argument" PR so I don't think I've broken that. I've had a try at fixing some of the dartanalyzer errors breaking all the builds BTW and most of them seem to be within my abilities except for a few tricky ones I'm not enough of a Dart pro for yet. I'd be happy to throw up a PR with the easy wins if you want though!

@nex3 nex3 self-requested a review April 6, 2021 20:49
@nex3
Copy link
Contributor

nex3 commented Apr 6, 2021

Thanks for the contribution! You're right that the test failures are unrelated; we should be resolving those soon. In the meantime, can you add an entry describing this change to the changelog?

@henrycatalinismith
Copy link
Contributor Author

henrycatalinismith commented Apr 7, 2021

Done! I've added it to the 1.32.9 section after having a look through the repo history and seeing that that seems to be A Thing here for teeny tiny changes like these when there are already new minor versions awaiting publishing. No trouble at all to move this into a new version of its own though if you'd prefer that instead! Can see there's a merge conflict incoming there with the minimum Chokidar version change so there's a good chance I'll be updating this branch again either way.

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.

None yet

2 participants