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

fix: removed the 10 white space infront of the issue's text #779

Closed
wants to merge 0 commits into from

Conversation

GaryThisSidee
Copy link

Resolves #772

@netlify
Copy link

netlify bot commented Sep 18, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit 02deb96
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/65104ce42e65880008c01bc1
😎 Deploy Preview https://deploy-preview-779--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

this doesn't seem to implement ignore-children like it was stated in the issue

@@ -398,7 +398,7 @@ const calculateRewardValue = (comments: Record<string, string[]>, incentives: In
const reward = wordReward.mul(value.map((str) => str.trim().split(" ").length).reduce((totalWords, wordCount) => totalWords + wordCount, 0));
sum = sum.add(reward);
} else {
if (!incentives.comment.elements[key]) {
if (!incentives.comment.elements[key] || incentives.comment.ignore_children.includes(key)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not desired functionality. this will only stop processing that particular item, but we want stop recursing when we encounter that item during parsing the markdown.
it basically replaces hardcoded variable:

const ItemsToExclude: string[] = [MarkdownItem.BlockQuote];

Copy link
Author

Choose a reason for hiding this comment

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

this is not desired functionality. this will only stop processing that particular item, but we want stop recursing when we encounter that item during parsing the markdown. it basically replaces hardcoded variable:

const ItemsToExclude: string[] = [MarkdownItem.BlockQuote];

ohhh my bad

Copy link
Member

Choose a reason for hiding this comment

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

it basically replaces hardcoded variable:

Shouldn't we remove the hardcoded variable if we can handle from the config @whilefoo? If so, would you mind creating a new issue for that?

Copy link
Author

Choose a reason for hiding this comment

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

it basically replaces hardcoded variable:

Shouldn't we remove the hardcoded variable if we can handle from the config @whilefoo? If so, would you mind creating a new issue for that?

should i remove it? will replace it with incentive ignore children

Copy link
Member

@0x4007 0x4007 Sep 20, 2023

Choose a reason for hiding this comment

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

I think it makes more sense to handle in a new issue if whilefoo thinks its necessary. That way this one can be merged in faster, most likely.

Copy link
Author

Choose a reason for hiding this comment

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

I think it makes more sense to handle in a new issue if whilefoo thinks its necessary. That way this one can be merged in faster, most likely.

sure

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we remove the hardcoded variable if we can handle from the config @whilefoo? If so, would you mind creating a new issue for that?

This issue is about this hardcoded variable, it stops recursing when it encounters one of the tags in this variable. So we just need to move it the config as ignore-children

Copy link
Collaborator

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

const traverse = (result: Record<string, string[]>, node: Node, itemsToExclude: string[]): Record<string, string[]> => {

you should modify this function to still parse the element but not its children. For example currently if it's ignore-children: code it won't include code item and its children, but we still want to include code. Don't forget to resolve conflicts

@pavlovcik do we want to set some sensible default like ignore-children: blockquote, code, pre, a

@0x4007
Copy link
Member

0x4007 commented Sep 24, 2023

Sensible defaults sounds like a good idea!

@GaryThisSidee
Copy link
Author

const traverse = (result: Record<string, string[]>, node: Node, itemsToExclude: string[]): Record<string, string[]> => {

you should modify this function to still parse the element but not its children. For example currently if it's ignore-children: code it won't include code item and its children, but we still want to include code. Don't forget to resolve conflicts

@pavlovcik do we want to set some sensible default like ignore-children: blockquote, code, pre, a

sure

@@ -9,6 +9,7 @@ type Node = {

const traverse = (result: Record<string, string[]>, node: Node, itemsToExclude: string[]): Record<string, string[]> => {
if (itemsToExclude.includes(node.nodeName)) {
result[node.nodeName].push(node.nodeName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will cause an error if nodeName is not in the object yet

if (node.childNodes && node.childNodes.length > 0) {

I recommend to just adding a condition here like !itemsToExclude.includes(node.nodeName)

@GaryThisSidee
Copy link
Author

@whilefoo going to open a new pull req and push the changes in one commit its kinda messy rn

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.

ignore-children comment incentive config
3 participants