Conversation
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 3 comments.
Reviewable status: 0 of 1 files reviewed, all discussions resolved.
.github/copilot-instructions.md line 89 at r1 (raw file):
- Use explicit true/false/null/undefined rather than truthy/falsy - Never rely on JavaScript's truthy or falsy. Instead, work with actual true, false, null, and undefined values, rather than relying on their interpretation as truthy or falsy. For example, if `count` might be null, or undefined, or zero, don't write code like `if (count)` or `const foo:string = someVariable ? 'a' : 'b'`. Instead, inspect for the null, undefined, or zero rather than letting the value be interpreted as truthy for falsy. For example, use `if (count == null)` or `const foo:string = someVariable != null 'a' : 'b'` or `if (count > 0)`.
I think I've preserved the intend of this list item while rewording it to say what it means to say. When I've asked agents to review changes I've sometimes had it claim I broke this rule, even if I didn't, or at least was in the spirit of it. I think as originally written it was too vague about what it was trying to prevent.
.github/copilot-instructions.md line 98 at r1 (raw file):
`const buildEvent: EventMetric | undefined = buildEvents[0];`. - Use `@if {}` syntax rather than `*ngIf` syntax. - Although interacting with existing code and APIs may necessitate the use of `null`, when writing new code, prefer using `undefined` rather than `null`.
I don't think this is a general rule to follow, and JSON doesn't have undefined, only null. I think it's more helpful to provide guidance regarding when to use each than to try to avoid one or the other.
.github/copilot-instructions.md line 100 at r1 (raw file):
- Although interacting with existing code and APIs may necessitate the use of `null`, when writing new code, prefer using `undefined` rather than `null`. - Fields that are of type Subject or BehaviorSubject should have names that end with a `$`. - When refactoring a method to be static, you should not need to start passing in `this` as an argument.
I could not figure out what this is supposed to mean. I'm guessing it's supposed to say something like don't refactor it if you still have to pass in this, or refactor it in such a way that you don't have to pass in this.
Though I would say if you're tempted to pass this in, it probably shouldn't be static, which makes me think this shouldn't be included here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3706 +/- ##
=======================================
Coverage 81.73% 81.73%
=======================================
Files 619 619
Lines 38651 38651
Branches 6317 6317
=======================================
Hits 31593 31593
- Misses 6084 6097 +13
+ Partials 974 961 -13 ☔ View full report in Codecov by Sentry. |
a0bb00b to
6e7d687
Compare
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Nateowami).
This change is