Skip to content
This repository has been archived by the owner on Aug 29, 2024. It is now read-only.

fix: avoid built-ins #34

Merged
merged 2 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/handlers/issue/comment-scoring-rubric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,17 @@ export class CommentScoring {
): (typeof CommentScoring.prototype.commentScores)[number]["details"][number]["wordScoreCommentDetails"] {
const wordScoreCommentDetails: { [key: string]: Decimal } = {};

const builtIns = new Set(
Object.getOwnPropertyNames(Object.prototype).filter((name) => typeof Object.prototype[name] === "function")
);

for (const word of words) {
let counter = wordScoreCommentDetails[word] || ZERO;
counter = counter.plus(this.roleWordScore);
wordScoreCommentDetails[word] = counter;
if (!builtIns.has(word)) {
let counter = wordScoreCommentDetails[word] || ZERO;
Copy link
Contributor

@molecula451 molecula451 Mar 14, 2024

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 type let counter: Decimal and test as such?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good observation

Copy link
Member Author

Choose a reason for hiding this comment

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

const counter: Decimal

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

Copy link
Contributor

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!

counter = counter.plus(this.roleWordScore);

wordScoreCommentDetails[word] = counter;
}
}

return wordScoreCommentDetails;
Expand Down
35 changes: 35 additions & 0 deletions src/handlers/tests/scoring-rubric-built-ins.test.ts
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),
Copy link
Contributor

@0x4007 0x4007 Mar 14, 2024

Choose a reason for hiding this comment

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

This doesn't make sense to me. was is not a built in property so why is it's count 0 if it's in your test array?

Copy link
Member Author

Choose a reason for hiding this comment

The 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;

Copy link
Contributor

Choose a reason for hiding this comment

The 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),
});
});
});
2 changes: 1 addition & 1 deletion src/utils/generate-configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export async function generateConfiguration(
const orgConfig = parseYaml(
await download({
repository: UBIQUIBOT_CONFIG_REPOSITORY,
owner: owner,
owner,
authenticatedOctokit,
})
);
Expand Down