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.

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

Replace empty interface with any type #4506

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Jan 2, 2024

Description:

For #3242

Checklist:

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

@coveralls
Copy link

coveralls commented Jan 2, 2024

Coverage Status

coverage: 64.544% (+0.003%) from 64.541%
when pulling ca81ae5 on Jacalz:any-interface-change
into 484a4bb on fyne-io:develop.

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.

Are you certain that these are interchangable and that this does not impact code usage at all?
i.e. if someone has var thingy interface{} and it is getting or setting something that is now any will the compiler translate every time without errors?

@Jacalz
Copy link
Member Author

Jacalz commented Jan 5, 2024

I'm pretty certain it's fine. It is supposed to be an alias for the other, i.e. var ListItemID = int allows the passes functions to use int as type.

andydotxyz
andydotxyz previously approved these changes Jan 5, 2024
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.

Ah yes an "=" alias - I missed that

@Jacalz
Copy link
Member Author

Jacalz commented Jan 6, 2024

Had to force-push a rebase to fix a merge conflict. Will likely require a re-approval. Sorry

@Jacalz Jacalz requested a review from andydotxyz January 6, 2024 17:07
@andydotxyz
Copy link
Member

Had to force-push a rebase to fix a merge conflict. Will likely require a re-approval. Sorry

This only happens if you rebase on top of conflicts though, merging in the develop branch allows the conflict to be resolved plainly in the new commit. Isn't that how we normally merge conflicts?

@Jacalz
Copy link
Member Author

Jacalz commented Jan 7, 2024

This only happens if you rebase on top of conflicts though, merging in the develop branch allows the conflict to be resolved plainly in the new commit. Isn't that how we normally merge conflicts?

Not for me. I have git set up to always rebase by default. I'm not a fan of merge commits

@Jacalz Jacalz merged commit a640a82 into fyne-io:develop Jan 7, 2024
11 checks passed
@andydotxyz
Copy link
Member

This only happens if you rebase on top of conflicts though, merging in the develop branch allows the conflict to be resolved plainly in the new commit. Isn't that how we normally merge conflicts?

Not for me. I have git set up to always rebase by default. I'm not a fan of merge commits

Is it possible to avoid them when conflicts are an issue? As you know force push does make the review much harder.

@Jacalz
Copy link
Member Author

Jacalz commented Jan 7, 2024

I can try to remember to force git to add a merge conflict in cases where review is ongoing. But is it really an issue in this case, or similar, where it already has been approved?

@Jacalz Jacalz deleted the any-interface-change branch January 7, 2024 18:54
@andydotxyz
Copy link
Member

As we would point out to other contributors a force-push (even if the PR has been approved) requires a full re-review as there is no trace of what has changed since the review

@Jacalz
Copy link
Member Author

Jacalz commented Jan 7, 2024

Yeah that's true. I'll have to try to remember to force git to do a merge commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants