-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
fix: fixes bug in category builder when word 'constructor' exists #564
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 01d99f5 in 38 seconds
More details
- Looked at
14
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. src/views/settings/CategoryBuilder.vue:223
:
- Assessed confidence :
100%
- Grade:
0%
- Comment:
The syntax used for initializing theMap
is incorrect. In JavaScript, theMap
should be initialized without type annotations directly in the constructor. Here's the correct way to initialize it:
const words = new Map();
- Reasoning:
The PR attempts to fix a bug related to the handling of the word 'constructor' in JavaScript objects. The author has replaced the plain object with a Map to avoid the issue where properties inherited from the object's prototype (like 'constructor') could interfere with the intended use of the object as a simple dictionary. However, the syntax used for initializing the Map is incorrect. The correct syntax for initializing a Map in JavaScript does not use the generic type declaration directly in the constructor but rather through the variable declaration.
Workflow ID: wflow_uz5WMWxvR5NINdHo
You can customize Ellipsis with review rules, user-specific overrides, quiet
mode, and more. See docs.
⌛ 12 days left in your free trial, upgrade for $20/seat/month or contact us.
01d99f5
to
6d10c19
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #564 +/- ##
=======================================
Coverage 25.60% 25.60%
=======================================
Files 27 27
Lines 1613 1613
Branches 281 273 -8
=======================================
Hits 413 413
- Misses 1141 1174 +33
+ Partials 59 26 -33 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 6d10c19 in 1 minute and 57 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
additional comments because they didn't meet confidence threshold of50%
.
Workflow ID: wflow_Av0NZSn3bneUWyYC
You can customize Ellipsis with review rules, user-specific overrides, quiet
mode, and more. See docs.
⌛ 12 days left in your free trial, upgrade for $20/seat/month or contact us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not the correct usage of a Map. in fact it is just the same as before, since the Map interface requires using dedicated methods like Map.has(), Map.get(), and Map.set().
so even with the patch, constructor' in words
will always evaluate to true.
instead it has to be checked with words.has('constructor')
and consequently all usage of this.words
would have to be adjusted as well
Ah right, dang it. Made the change in #565, not sure if it's finally correct as I haven't tested. |
Fixes ActivityWatch/activitywatch#1054
Summary:
This PR fixes a bug in
CategoryBuilder.vue
by changing the storage of words from an object to aMap
, ensuring all words, including JavaScript property names like 'constructor', are treated uniformly.Key points:
CategoryBuilder.vue
related to handling of the word 'constructor'words
from an object to aMap
to avoid prototype issuesGenerated with ❤️ by ellipsis.dev