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

Add scrubber POC #3

Merged
merged 7 commits into from
Mar 26, 2021
Merged

Add scrubber POC #3

merged 7 commits into from
Mar 26, 2021

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Mar 15, 2021

Notion ticket link

Bootsrap CLI - POC of scrubber

Implementation description

  • Implement scrubber tool that allows for removing lines in between configurable tags
  • Current tool operates on all files in specified directories
  • Opening tag spec: tag-name {
  • Closing tag spec: } tag-name

Steps to test

  1. Open test file in scrubber/test_dir/ and verify lines to be removed
  2. Run yarn dev
  3. Verify lines in between tags have been removed

What should reviewers focus on?

  • scrubber.ts
  • scrubberConfig.json

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@xinhaoz xinhaoz changed the title Xin/scrubber Add scrubber POC Mar 15, 2021
package.json Show resolved Hide resolved
Copy link
Member

@sherryhli sherryhli left a comment

Choose a reason for hiding this comment

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

Awesome stuff! only required change is a Windows line ending issue fix

async function scrubFile(
filePath: string,
tags: TagNameToAction,
isDryRun: boolean,
Copy link
Member

Choose a reason for hiding this comment

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

Nice, this is helpful 🔥

scrubber/scrubber.ts Outdated Show resolved Hide resolved
scrubber/scrubber.ts Outdated Show resolved Hide resolved
Comment on lines +11 to +12
await scrubber.parseConfig("scrubber/scrubberConfig.json");
await scrubber.start(actions);
Copy link
Member

Choose a reason for hiding this comment

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

To confirm my understanding, default tag settings go in the config file, and results from CLI user prompts are passed as the actions argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that is how it is currently set up.
Alternatively we can just pass all tags through the function and not put any in the config. Thoughs? @alexguo8

Copy link
Contributor

Choose a reason for hiding this comment

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

The CLI handles default arguments so if that would reduce the complexity of the code by a lot then I agree we can just pass all tags through the CLI
However both are fine for me

@sherryhli
Copy link
Member

Oh additional question, are we handling comments preceding tags here or in a separate PR?

alexguo8
alexguo8 previously approved these changes Mar 19, 2021
Copy link
Contributor

@alexguo8 alexguo8 left a comment

Choose a reason for hiding this comment

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

Looks good to me

sherryhli
sherryhli previously approved these changes Mar 19, 2021
Copy link
Member

@sherryhli sherryhli left a comment

Choose a reason for hiding this comment

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

🚢

@xinhaoz xinhaoz dismissed stale reviews from sherryhli and alexguo8 via dc9fbe8 March 26, 2021 01:11
@xinhaoz xinhaoz merged commit c705f9f into main Mar 26, 2021
@xinhaoz xinhaoz deleted the xin/scrubber branch March 26, 2021 01:18
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.

3 participants