-
Notifications
You must be signed in to change notification settings - Fork 67
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
fix: remove comments on config values #2383
base: main
Are you sure you want to change the base?
Conversation
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🔴 | Statements | 2.71% | 118/4350 |
🔴 | Branches | 2.93% | 87/2968 |
🔴 | Functions | 1.33% | 19/1430 |
🔴 | Lines | 2.67% | 114/4268 |
Test suite run success
32 tests passing in 4 suites.
Report generated by 🧪jest coverage report action from 20d0818
// overflow: 'auto', | ||
alignItems: 'center', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this. When the announcement has multiple lines, it's better to place a close button at the top instead of the middle.
const uncommentedValue = | ||
typeof valueObj.value === 'string' && valueObj.value.includes('#') | ||
? valueObj.value.split('#')[0].trim() | ||
: ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good idea because the value can have a #.
some="This#Value#has#"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the current TOML parser does not support inline comments, please search for an alternative one that supports inline comments.
2f4edcd
to
20d0818
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @ironAiken2 and the rest of your teammates on Graphite |
This PR Resolves #2382 Issue. (and minor fix on summary page's alert margin)
Why the bug occurred
Inside the config.toml file, a description of each value is included as a comment. These comments were being read together to prevent the default value from being used even when no value was provided.
Feature
Checklist: (if applicable)