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

Rebased qgis fixes #516

Merged
merged 14 commits into from
Oct 14, 2023
Merged

Rebased qgis fixes #516

merged 14 commits into from
Oct 14, 2023

Conversation

luis-pereira
Copy link
Member

A rebase of #499 PR

@yan12125
Copy link
Member

yan12125 commented Oct 9, 2023

Looks good overall. Just curious - is it possible to get rid of assertWithSideEffect by changing some member functions to const?

@luis-pereira
Copy link
Member Author

Looks good overall. Just curious - is it possible to get rid of assertWithSideEffect by changing some member functions to const?

Sorry for the long delay.
It's possible and done with commit 634e66e.

@yan12125 yan12125 merged commit 7f501f5 into master Oct 14, 2023
2 checks passed
@yan12125 yan12125 deleted the rebased_qgis_fixes branch October 14, 2023 08:48
@yan12125
Copy link
Member

Thank you! I merge this without squashing as those changes are kind of independent, and thus squashing may make git blame harder.

Sorry for the long delay.

A few days is short for me :)

@luis-pereira
Copy link
Member Author

Thank you! I merge this without squashing as those changes are kind of independent, and thus squashing may make git blame harder.

It would make history a lot harder to understand.

@yan12125
Copy link
Member

In my opinion, this is a good trade-off - easier debugging (ex: via git bisect) is more important than simpler history.

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.

7 participants