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

fix: avoid built-ins #34

merged 2 commits into from
Mar 14, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Mar 14, 2024

Resolves #31

  • Built an exclusion list based on object prototype built-ins

QA:

  • difficult to QA in a working sense other than below

image

(async () => {
  const wordScoreCommentDetails: { [key: string]: Decimal } = {};
  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)) {
      let counter = wordScoreCommentDetails[word] || ZERO;
      wordScoreCommentDetails[word] = counter;
    }
  }

  console.log("scoringDetails: ", wordScoreCommentDetails);
})().catch(console.error);

@0x4007
Copy link
Member

0x4007 commented Mar 14, 2024

You can definitely include my.ts as a jest test. Just name it the same as the filename like *.test.ts

@molecula451
Copy link
Contributor

the only viable way i'd see testing this in a production like env is keyring setting up basically everything from scratch which would take more time than what the fix should be accomplished

I would merge this a HOT-fix to check the reproduction, it doesn't do then a revert back pretty much won't break any other stuff, i recall originally pavlovcik asked a core dev to have a test env for similar situations but not sure if it was built

}

expect(wordScoreCommentDetails).toEqual({
was: new Decimal(0),
Copy link
Member

@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
Member

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

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
Member

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!

@0x4007 0x4007 merged commit 90d7570 into ubiquibot:main Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: counter.plus is not a function
3 participants