Skip to content
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

refactor(markdown-docx): export getClass as helper - #397 #414

Closed
wants to merge 1 commit into from

Conversation

K-Kumar-01
Copy link
Collaborator

@K-Kumar-01 K-Kumar-01 commented Jun 18, 2021

Signed-off-by: k-kumar-01 kushalkumargupta4@gmail.com

Need: While working on LIST_RULE, there was requirement of node.$class. Since a function has already been created in transformer, the decision was to export it as a helper and use it.

Changes

  • use getClass as helper instead of transformer class function

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to master from fork:branchname

use getClass as helper instead of transformer class function

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>
Copy link
Member

@algomaster99 algomaster99 left a comment

Choose a reason for hiding this comment

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

Although a nice initiative, I disagree with this design. See comment.

Comment on lines +99 to +107
/**
* Gets the class of a given CiceroMark node.
*
* @param {object} node CiceroMark node entity
* @returns {string} Class of given node
*/
function getClass(node) {
return node.$class;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would advise against this because getClass is supposed to be bound to the class as you will have access to "node" only in the transformer class, so it doesn't make sense to expose it. Moreover, helper methods don't have such strict parameter requirement. For example, sanitizeHtmlChars is a perfect example of a helper method because it can be a static method too as it just needs a string and doesn't matter from where we invocate it. However, getClass depends strongly on the input because of the $class of node and you will only get this while iterating over CiceroMark. So it's better to keep this as an instance method only.

To summarise, helper methods are those functions that are not much dependent upon the state of the application and can be reused throughout the code. You can also think of them as static methods in a class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@algomaster99 Shall I close this PR then?

Copy link
Member

Choose a reason for hiding this comment

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

If you are with my stand, then yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@algomaster99 Yeah, I think it makes sense but I have one doubt. So we want node.$class in the list transformer.
Should any function returns a node.$class or we write it as node.$class in the comparison itself?
We currently use this.getClass in the transformer itself and the rule will be imported from another file.

Copy link
Member

Choose a reason for hiding this comment

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

Try to decouple rules from the node object. I feel there should be a way.

@K-Kumar-01 K-Kumar-01 closed this Jun 21, 2021
@jeromesimeon jeromesimeon deleted the k-kumar-01/i397/refactor-getClass branch September 27, 2021 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants