FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

rules: add `watch` subsystem by MoLow · Pull Request #103 · nodejs/core-validate-commit · GitHub

rules: add watch subsystem - #103

Merged
Trott merged 1 commit into
nodejs:mainfrom
MoLow:main
Nov 7, 2022
Merged

rules: add watch subsystem#103
Trott merged 1 commit into
nodejs:mainfrom
MoLow:main

Conversation

MoLow commented Nov 3, 2022

Copy link
Copy Markdown
Member

No description provided.

codecov Bot commented Nov 3, 2022
edited
Loading

Copy link
Copy Markdown

Codecov Report

Base: 93.93% // Head: 93.93% // No change to project coverage 👍

Coverage data is based on head (49fb56f) compared to base (fa47c55).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #103   +/-   ##
=======================================
  Coverage   93.93%   93.93%           
=======================================
  Files          19       19           
  Lines         462      462           
=======================================
  Hits          434      434           
  Misses         28       28           
Impacted Files Coverage Δ
lib/rules/subsystem.js 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

MoLow commented Nov 5, 2022

Copy link
Copy Markdown
Member Author

@nodejs/tsc who can approve this?

Trott commented Nov 5, 2022

Copy link
Copy Markdown
Member

I guess this most impacts @nodejs/releasers?

Trott commented Nov 5, 2022

Copy link
Copy Markdown
Member

Not sure how I feel about the underbar. The only place that appears otherwise is in subsystem strings that correspond to modules that have an underbar in their name. Any reason this isn't just watch? For that matter, might it just be fine using cli or something like that? Does it need a new subsystem?

Non-blocking questions for discussion. If other people think it's fine or if this has already been discussed and I just missed it, feel free to land without engaging on this. :-D

MoLow commented Nov 6, 2022

Copy link
Copy Markdown
Member Author

the underbar is not very important - we can change to watch.
there are a couple of PR's related to watch mode that I dont think fit the cli subsystem:
nodejs/node#45259
nodejs/node#45271

RafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm +1 for watch

MoLow requested a review from RafaelGSS November 6, 2022 11:58

Trott commented Nov 6, 2022

Copy link
Copy Markdown
Member

Sorry to be a pain about this, but can you squash and rewrite the commit message? I want to make sure that if we add the commit-queue label to this, the commit message says watch and not watch_mode.

Trott commented Nov 6, 2022
edited
Loading

Copy link
Copy Markdown
Member

We'll have to be careful to make sure that people don't use this for the fs module's watcher functionality, but I think that's a pretty unlikely error for someone to make and that reviewers would catch the error, so I'm 👍 on this.

MoLow commented Nov 6, 2022

Copy link
Copy Markdown
Member Author

@Trott done. can this be merged?

GeoffreyBooth changed the title rules: add watch_mode subsystem rules: add watch subsystem Nov 6, 2022
Trott merged commit bfadfb0 into nodejs:main Nov 7, 2022

Trott commented Nov 7, 2022

Copy link
Copy Markdown
Member

@Trott done. can this be merged?

Whoops, forgot that we use the commit button in this repo so I guess I could have done it right in the interface. Anyway, yes, merged, thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

5 participants


Back | FazBrowse Home | New Git URL