-
Notifications
You must be signed in to change notification settings - Fork 12
fix: avoid built-ins #34
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import Decimal from "decimal.js"; | ||
|
||
describe("Scoring Rubric Built-ins", () => { | ||
let wordScoreCommentDetails: { [key: string]: Decimal }; | ||
|
||
beforeEach(() => { | ||
wordScoreCommentDetails = {}; | ||
}); | ||
|
||
it("should increment counter for non-built-in words", () => { | ||
const words = ["hasOwnProperty", "valueOf", "was", "async", "but", "the", "constructor", "isn", "t", "I", "was"]; | ||
const ZERO = new Decimal(0); | ||
|
||
const builtIns = new Set( | ||
Object.getOwnPropertyNames(Object.prototype).filter((name) => typeof Object.prototype[name] === "function") | ||
); | ||
|
||
for (const word of words) { | ||
if (!builtIns.has(word)) { | ||
const counter = wordScoreCommentDetails[word] || ZERO; | ||
wordScoreCommentDetails[word] = counter; | ||
} | ||
} | ||
|
||
expect(wordScoreCommentDetails).toEqual({ | ||
was: new Decimal(0), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't make sense to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because the in the test I've simplified the logic down to the core issue and removed all of the actual scoring, so it's set as zero by default. All of the built-ins are removed from the array of words const counter = wordScoreCommentDetails[word] || ZERO; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand but based on the non test code changes, and moleculas recommendation I can merge this pull |
||
async: new Decimal(0), | ||
but: new Decimal(0), | ||
the: new Decimal(0), | ||
isn: new Decimal(0), | ||
t: new Decimal(0), | ||
I: new Decimal(0), | ||
}); | ||
}); | ||
}); |
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.
ZERO
it's still constructed as a Decimal, can you perhaps so it's expect a decimal in the end, can you perhaps typelet counter: Decimal
and test as such?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.
Good observation
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.
It's already inferred as such without being explicit but the test still passes either way, I have just tried locally but with the PR being closed it's a dud.
Proved to be working via rpc-handler anyway happy days
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.
perhaps for a future improved, pavlovick is one of the like loves everything
Typed
!