refactor(formatter): Move general utility functions from YScope formatter to global utilities. #190
Conversation
WalkthroughThis pull request refactors the handling of JSON value conversions and string escape formatting in the YscopeFormatter module. It updates import paths in the RoundFormatter and main YscopeFormatter files, reorganises related documentation and warning messages, and removes several utility functions from the local utils file. A new util module is introduced under Changes
Sequence Diagram(s)sequenceDiagram
participant RF as RoundFormatter
participant FU as Formatters Utils
RF->>+FU: Call jsonValueToString(jsonValue)
FU-->>-RF: Return formatted string
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…iewer-fork into formatterRefactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/formatters.ts (2)
16-18: Consider using a more descriptive function name.The function name
removeBackslashcould be more descriptive to indicate that it only removes single backslashes, not double backslashes.-const removeBackslash = (str: string): string => { +const removeSingleBackslash = (str: string): string => {
67-69: Improve readability by following coding guidelines.According to the coding guidelines, prefer
false == <expression>rather than!<expression>.- return "object" === typeof input ? + return false == ("object" === typeof input) ? + String(input) : + JSON.stringify(input);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/services/formatters/YscopeFormatter/FieldFormatters/RoundFormatter.ts(1 hunks)src/services/formatters/YscopeFormatter/index.ts(3 hunks)src/services/formatters/YscopeFormatter/utils.ts(1 hunks)src/utils/formatters.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/services/formatters/YscopeFormatter/FieldFormatters/RoundFormatter.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/formatters/YscopeFormatter/index.tssrc/utils/formatters.tssrc/services/formatters/YscopeFormatter/utils.ts
🔇 Additional comments (4)
src/utils/formatters.ts (1)
73-77: LGTM! Good job on the exports.The exports are well-organized and only expose the necessary functions.
src/services/formatters/YscopeFormatter/utils.ts (1)
9-12: LGTM! Good job on organizing imports.The imports are well-organized and correctly reference the new global utilities.
src/services/formatters/YscopeFormatter/index.ts (2)
34-35: LGTM! Clear and concise warning message.The warning message is now more concise and easier to understand.
67-70: LGTM! Documentation improvements.The documentation comments are now more accurate and consistent with the codebase terminology.
junhaoliao
requested review from
junhaoliao
and removed request for
junhaoliao
There was a problem hiding this comment.
correct me if i'm wrong with any below:
my understanding of why we want to move some util functions into src/utils/formatter.ts is because some functions, previously only needed for formatting, are now needed by decoding in a future PR.
however:
- Most utils in
src/utilsare generic in the sense that they can be used by both client and server code (action.tsandstate.tsseem to be exceptions). - For utils that are used in key parsing, i feel they can be better classified as "decoder" utils than "formatter" utils.
src/services/decodersandsrc/services/formattersare inter-dependent at this moment, and entirely decoupling them from each other does not give immediate benefits.
how about:
- We create
src/services/decoders/utils.tsand place the utils related to key parsing inside. - For the utils that are specific to formatting, we leave them at their original places.
How do feel about creating src/utils/parsing.ts specifically for the regex code? If you disagree with this. I would prefer to just leave in formatter/utils (and close this PR), and import to decoder/utils. I feel like existing comments in other parts of formatter codebase, make it easier to read these utils. I feel like they would be more out of place in decoder/utils. |
i agree
sure |
Description
This is refactoring PR (no functionality changes) in advance of adding support for hierarchical filter keys. PR moves some functions from
YscopeFormatter/utils.tsto new fileutils/formatters.ts. These functions will be used in future for hierarchical filter keys in decoder files (see WIP), so moving them into global utils. It is better style to import them from global utils than from YscopeFormatter. Code is untouched, there are minor comment tweaks.Checklist
breaking change.
Validation performed
Tested a format string, everything looked okay
Summary by CodeRabbit
These changes are internal improvements and do not impact the app’s visible functionality.