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

[EXTERNAL] Improved checking for import, moved removeComments #1765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sagarishere
Copy link
Contributor

Improved checking of imports and moved removeComments to separate function, clearly explaining that it's limited to javascript type of comments.

Why?

many words like important etc got banned due to include import statement.
moving removeComments to separate function as it is a separate concern
separating removeComments to prepare it for language detection with arguments in the future

Solution Overview

Will improve removeComments after this commit is accepted as do not want to push too much in a single commit.
Will set default parameter of language to Javascript so that it does not break the current code, as well as define treatment of bash files (for example they have '#' comments) separately.

Implementation Details

Explained above.

Copy link
Contributor Author

@sagarishere sagarishere left a comment

Choose a reason for hiding this comment

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

Trying best to get the commit approved as it is really a very slow process.

}
return { code: sanitizedCode, rawCode, path }
} catch (error) {
console.error(error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the maintainer may replace the console.log(error) with fatal(error)
as fatal does precisely that plus whatever is required to be done on top of it

}

const removeComments = (code) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the maintainer may replace this as const removeComments = (code, lang='JavaScript') =>
this helps in further direction to improve implementation

const removeComments = (code) => {
// removes JS single line and multi-line comments only. Not for bash files etc.
// for use with multiple file-types, I suggest writing a removeComments function with language-type as input and then handling accordingly
return code.replace(/\/\*[\s\S]*?\*\/|\/\/.*/g, "").trim()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a condition could be added, something like if (lang === 'JavaScript') || (lang === 'JS') {

@HarryVasanth HarryVasanth changed the title Improved checking for import, moved removeComments [EXTERNAL] Improved checking for import, moved removeComments Jul 8, 2024
@HarryVasanth HarryVasanth added 📕 JS JavaScript 🧪 test Tests labels Nov 21, 2024
Improved checking of imports and moved removeComments to separate function, clearly explaining that it's limited to javascript type of comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📕 JS JavaScript 🧪 test Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants