-
Notifications
You must be signed in to change notification settings - Fork 653
BREAKING(ini): parse understands booleans, undefined, null and numbers #6121
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6121 +/- ##
==========================================
- Coverage 96.57% 96.48% -0.10%
==========================================
Files 535 538 +3
Lines 40583 40974 +391
Branches 6094 6165 +71
==========================================
+ Hits 39195 39534 +339
- Misses 1344 1396 +52
Partials 44 44 ☔ View full report in Codecov by Sentry. |
ini/_ini_map.ts
Outdated
: (_key, value, _section) => value; | ||
: (_key, value, _section) => { | ||
if (value === "undefined") return undefined; | ||
if (!isNaN(+value) && !value.includes('"')) return parseInt(value); |
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.
Why not just use !isNaN(value)
?
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.
There's no particular reason, I just put it that way out of habit
Edit: I just checked and isNaN expects a number, so there's no way to use !isNaN(value)
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 allows expression like 1e3
as parsed as number, and it results 1
. Is that intentional?
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.
Not at all, but its because the parseInt function just returns the first complete integer that it finds on the string. So IsNaN says that value
is a number but parseInt can just parse the 1, but easy fix
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.
Edit: I just checked and isNaN expects a number, so there's no way to use
!isNaN(value)
Oh you're right. I forgot we're using typescript not javascript lol
Edit: BTW I'm curious about the check !value.includes('"')
. What is it used for?
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 its a number but has a "
the user wants as a string and not as a number
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.
Fair enough
Can you also work on |
yeah sure no problem |
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.
I think these value interpretations are fine because they are very natural in JavaScript, and I guess they probably gave least surprise to the users. LGTM
Note: There's no official specifications for INI format, and major parsers are very incompatible with each other. So we need to choose one way of parsing them, and I find this one is enough reasonable.
AFAIK, this functionality is all that remains for stabilising |
Discussed this internally and we'd like to drop the interpretation of What do you think? |
thats ok, this comes closer to what |
Ok. Let's go with this |
Closes #4806