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.

Sign up for GitHub

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

Jump to bottom

move tokenizer step to teragrep package and rename #400

Merged
merged 3 commits into from
Nov 4, 2024
Merged

move tokenizer step to teragrep package and rename #400

merged 3 commits into from
Nov 4, 2024

Conversation

elliVM
Copy link
Contributor

@elliVM elliVM commented Oct 30, 2024

  • move tokenizer step to teragrep package
  • rename to TeragrepTokenizerStep
  • remove unused interface and logger

@elliVM elliVM self-assigned this Oct 30, 2024
@elliVM elliVM requested a review from 51-code October 30, 2024 12:09
@elliVM elliVM marked this pull request as draft October 30, 2024 12:10
@elliVM elliVM marked this pull request as ready for review October 30, 2024 12:12
51-code
51-code approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@51-code 51-code left a comment

Choose a reason for hiding this comment

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

LGTM. I'll approve, but two things:

  1. TokenizerStep was renamed to TeragrepTokenizerStep but the AbstractTokenizerStep wasn't renamed in the same fashion, mistake or not? Either way the naming seems good to me.
  2. TokenizerStep should be refactored to multiple objects based on the enum tokenizerFormat, new issue about that.

@elliVM elliVM requested a review from kortemik November 1, 2024 13:37
@kortemik kortemik merged commit 81e59e0 into teragrep:main Nov 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kortemik kortemik kortemik approved these changes

@51-code 51-code 51-code approved these changes

Assignees

@elliVM elliVM

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elliVM @kortemik @51-code