Skip to content

Conversation

@gukoff
Copy link
Contributor

@gukoff gukoff commented Mar 13, 2021

The previous implementation had 3 bugs:

Also the logic itself was "magical": it removed 4 lines at a time, starting from a middle line but if possible (never successfully) from a line before that has an exception start in it...

This change should fix all these problems.

The previous implementation had 3 bugs:
- IndexError when only 1 line in error_string;
- infinite loop when repeatedly can't strip down error_string;
- ", ln" instead of ", in" in one matching expression that rendered it useless.

Also the logic itself was "magical": it removed 4 lines at a time, starting from a middle line but if possible (never successfully) from a line before that has an exception start in it...

This change should fix all these problems.
@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Mar 13, 2021
@ghost
Copy link

ghost commented Mar 13, 2021

Thank you for your contribution gukoff! We will review the pull request and get back to you soon.

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 13, 2021

feedback

@jiasli jiasli changed the title Fix feedback minification. {Feedback} Fix feedback minification Mar 15, 2021
@jiasli
Copy link
Member

jiasli commented Mar 15, 2021

@gukoff, first thanks for the careful work.

Actually, we are considering removing the feedback minification logic, as it has caused us many problems (#16386).

Also, given modern browsers are already accepting URL length exceeding the old limitation (https://stackoverflow.com/a/417184):

the motivation to minify the error_string seems less reasonable.

@gukoff
Copy link
Contributor Author

gukoff commented Mar 15, 2021

If you're not doing it before the next release, this fix still makes sense :)

I agree that minification needs a revamp - large parts of code seem unnecessary and not tested.

The URL length limit is still there, it's just usually higher than 2KB. E.g. the (undocumented?) GitHub's serverside limit on URL length is 8KB.

Perpahs minification could always strip the prefix of the error message.

Also, after the minification (or instead of it) we can ask the user in the CLI if opening the issue was successful. If not, write the manual instructions to the terminal: "1. open this page", "2. copy-paste this text".


About GitHub serverside limit:

image
example URL

@jiasli
Copy link
Member

jiasli commented Mar 15, 2021

If you're not doing it before the next release, this fix still makes sense :)

Of course. But for the time being I really don't have enough time to go through your code thoroughly. 😥

Also, after the minification (or instead of it) we can ask the user in the CLI if opening the issue was successful. If not, write the manual instructions to the terminal: "1. open this page", "2. copy-paste this text".

Yes, we do have this logic. I will make it more user-friendly.

@jiasli jiasli self-assigned this Mar 18, 2021
@jiasli jiasli merged commit b1e86d6 into Azure:dev Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants