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

Refactor: fixing (potential) bugs around closing #1995

Merged
merged 7 commits into from Mar 3, 2021
Merged

Refactor: fixing (potential) bugs around closing #1995

merged 7 commits into from Mar 3, 2021

Conversation

tehsphinx
Copy link
Contributor

@tehsphinx tehsphinx commented Feb 21, 2021 •
edited

Description:

There was one http body close missing.

The other fixes resolve around ignoring the error on writable file handler closing (which should not be done). An error might be returned from Close that indicates a write failure. For more details see https://www.joeshaw.org/dont-defer-close-on-writable-files/

Checklist:

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

@Jacalz
Copy link
Member

Jacalz commented Feb 21, 2021

I have been meaning to do this for a while now. Thank you very much for getting this sorted 👍

@tehsphinx tehsphinx marked this pull request as ready for review February 21, 2021 20:32
@tehsphinx
Copy link
Contributor Author

I have been meaning to do this for a while now. Thank you very much for getting this sorted 👍

I was merely following some linter issues. gosec was complaining about file closing without error checks. Then I started to stumble over more stuff. I don't think this is complete. There probably is more.

@Jacalz
Copy link
Member

Jacalz commented Feb 21, 2021 •
edited

I have been meaning to do this for a while now. Thank you very much for getting this sorted 👍

I was merely following some linter issues. gosec was complaining about file closing without error checks. Then I started to stumble over more stuff. I don't think this is complete. There probably is more.

I know. That’s exactly what I have been intending to do for quite some time 🙂

@andydotxyz
Copy link
Member

@Jacalz from your comments I assumed you would be best placed to review this.
Just say if you would prefer that I do?

@Jacalz
Copy link
Member

Jacalz commented Feb 26, 2021

@Jacalz from your comments I assumed you would be best placed to review this.
Just say if you would prefer that I do?

Please go ahead. I have a lot to do at the moment.

andydotxyz
andydotxyz requested changes Mar 1, 2021
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.

A lot of good catches here, thanks.
However many of the "defer returned" errors are not returned as their values are shadowed by local variables.

app/preferences.go Show resolved Hide resolved
cmd/fyne/internal/commands/bundle.go Show resolved Hide resolved
cmd/fyne/internal/commands/bundle.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/bundle.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/package-darwin.go Show resolved Hide resolved
cmd/fyne/internal/commands/package-darwin.go Show resolved Hide resolved
cmd/fyne/internal/commands/release.go Outdated Show resolved Hide resolved
cmd/fyne/internal/util/file.go Show resolved Hide resolved
andydotxyz
andydotxyz previously approved these changes Mar 2, 2021
View reviewed changes
fpabl0
fpabl0 reviewed Mar 2, 2021
View reviewed changes
Copy link
Member

@fpabl0 fpabl0 left a comment

Choose a reason for hiding this comment

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

Some comments about named returns and simplification.

cmd/fyne/internal/util/file.go Show resolved Hide resolved
cmd/fyne/internal/util/file.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/release.go Show resolved Hide resolved
fpabl0
fpabl0 suggested changes Mar 3, 2021
View reviewed changes
Copy link
Member

@fpabl0 fpabl0 left a comment

Choose a reason for hiding this comment

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

Based on the last commit, I think that change should be applied to the remaining defer functions too.

app/preferences.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/package-darwin.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/package-darwin.go Outdated Show resolved Hide resolved
cmd/fyne/internal/commands/release.go Outdated Show resolved Hide resolved
fpabl0
fpabl0 approved these changes Mar 3, 2021
View reviewed changes
Copy link
Member

@fpabl0 fpabl0 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks :)

andydotxyz
andydotxyz approved these changes Mar 3, 2021
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.

Thanks.
And thanks for educating us on some of the defer and return code.
Glad to hear they are considering removing naked returns :)

@andydotxyz andydotxyz merged commit 5c4719b into fyne-io:develop Mar 3, 2021
@tehsphinx tehsphinx deleted the bug/closing branch March 4, 2021 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andydotxyz andydotxyz andydotxyz approved these changes

@fpabl0 fpabl0 fpabl0 approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@tehsphinx @Jacalz @andydotxyz @fpabl0