Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix panic on invalid file URI #3275 #3308

Merged
merged 5 commits into from
Oct 21, 2022
Merged

Fix panic on invalid file URI #3275 #3308

merged 5 commits into from
Oct 21, 2022

Conversation

ChandanChainani
Copy link
Contributor

@ChandanChainani ChandanChainani commented Oct 4, 2022 •
edited by Jacalz
Loading

Description:

Fixes #3275

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@coveralls
Copy link

coveralls commented Oct 4, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.004%) to 61.394% when pulling d5d7db5 on ChandanChainani:fix/3275/invalid_file_uri into 94713bb on fyne-io:develop.

@Jacalz
Copy link
Member

Jacalz commented Oct 4, 2022

Thanks for the PR. You have created the PR against the master branch but the PR template states that it should target develop. Would you mind changing the target branch to be correct?

@Jacalz
Copy link
Member

Jacalz commented Oct 5, 2022

Also, please add "Fixes #3275" to the description. This will close the issue automatically when the PR is merged

@Jacalz Jacalz mentioned this pull request Oct 5, 2022
Closed
7 tasks
@ChandanChainani ChandanChainani changed the base branch from master to develop October 5, 2022 06:39
@ChandanChainani
Copy link
Contributor Author

ChandanChainani commented Oct 5, 2022 •
edited
Loading

@Jacalz Updated as per comments

Jacalz
Jacalz requested changes Oct 5, 2022
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have one comment on making the code more readable.

storage/repository/parse.go Outdated Show resolved Hide resolved
Jacalz
Jacalz reviewed Oct 5, 2022
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be good if you could rebase on develop given that #3301 landed recently and fixed another crash.

@ChandanChainani ChandanChainani force-pushed the fix/3275/invalid_file_uri branch from badf620 to 3a19bcf Compare October 5, 2022 17:03
@ChandanChainani ChandanChainani requested a review from Jacalz October 5, 2022 17:05
Jacalz
Jacalz requested changes Oct 7, 2022
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that it would be good to have an actual test case for Parse (in the right place) that tests when an error should happen. The new code path is never tested to actually be working unfortunately

@ChandanChainani ChandanChainani requested a review from Jacalz October 8, 2022 06:56
@Jacalz Jacalz requested a review from Bluebugs October 8, 2022 09:02
andydotxyz
andydotxyz approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks

@andydotxyz
Copy link
Member

@Jacalz there is still an outstanding change request from you but I think it's good now.

Jacalz
Jacalz approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Looks good. Thanks for working on this :)

@Jacalz Jacalz merged commit 25bfeda into fyne-io:develop Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jacalz Jacalz Jacalz approved these changes

@andydotxyz andydotxyz andydotxyz approved these changes

@Bluebugs Bluebugs Awaiting requested review from Bluebugs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash parsing invalid file URI
4 participants
@ChandanChainani @coveralls @Jacalz @andydotxyz