-
Notifications
You must be signed in to change notification settings - Fork 234
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
feature: refactor get the snippet code #1120
feature: refactor get the snippet code #1120
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
.then(response => response.text()) | ||
.then(text => { | ||
const parsed = JSON.parse(JSON.parse(text).jsonPayload); |
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.
would this work?
.then(response => response.text()) | |
.then(text => { | |
const parsed = JSON.parse(JSON.parse(text).jsonPayload); | |
.then(response => response.json()) | |
.then(jsonData => { | |
const parsed = JSON.parse(jsonData.jsonPayload); |
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.
@yedpodtrzitko I think keeping my version will work fine. As for yours, I'm not sure.
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.
We should try as it is better ? can you check @NguyenTuanCanh ?
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.
Both should work, one needs one less JSON.parse call.
} | ||
if (line.toLowerCase().includes(lowerTerm)) { | ||
foundLine = index; | ||
startingLine = Math.max(foundLine - 5, 0); |
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.
Without the break, the code might not early exit anymore, why this change ?
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.
@sebavan Wow, thank you for your question. Updated!
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.
Your version is still different as foundLine will not be set outside of the condition but was always set in the loop version. I would in this case recommend to keep the loop as there is no reasons not too ?
d418fbc
to
3445d40
Compare
This is mainly a cosmetic change, right? No functionality has changed? |
3445d40
to
23c74da
Compare
I am a bit confused as to the current changes. Formatting has changed a bit, but it seems like the code as it is is almost (or even completely) the same? |
any update here? or should I close it? |
Changes made: