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

Changed float to double in Widgets #2374

Merged
merged 6 commits into from Jan 12, 2024

Conversation

charlenni
Copy link
Member

Changed all floats to doubles in Widgets. Now the values are converted at the end when they used for SkiaSharp.

@charlenni charlenni changed the title Changed float to double in Widgets [WIP] Changed float to double in Widgets Dec 30, 2023
@charlenni
Copy link
Member Author

@pauldendulk I now made small steps with many different commits, so that it is possible to see each step in the process. It isn't all, that was changed before, but I want to not invest more time before you say, that it is possible to review these PRs.

The next two PRs are here and here.

Do you think it is possible to review it in these small parts? Or do you don't except any changes?

Copy link
Member

@pauldendulk pauldendulk left a comment

Choose a reason for hiding this comment

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

Wonderful! This is very easy to review even though there are still 16 files and more than 100 changed lines. I have no remarks but did go over all changes.

This kind of change fits well in the phase where we are in now. Get basic things straightened out that you don't want to change when working on a specific feature.

@pauldendulk
Copy link
Member

This PR is 'draft' but if you don't see something that should be done yourself you could remove the draft. In that case I could have merged it right now.

I did not look at the upcoming PRs yet.

@charlenni charlenni changed the title [WIP] Changed float to double in Widgets Changed float to double in Widgets Dec 31, 2023
@charlenni charlenni marked this pull request as ready for review December 31, 2023 10:55
@charlenni
Copy link
Member Author

Replaced by #2377

@charlenni charlenni closed this Jan 2, 2024
@pauldendulk
Copy link
Member

@charlenni This is a wonderful one topic PR, yet it is close and added to a PR with many more changes. Do you agree that in hindsight this what was a bad idea, or is there another reason to work like this? If so, I would like to understand why.

@charlenni
Copy link
Member Author

@pauldendulk For me it is a timing problem. I have dozens of idea, but could realize only the first. Then I have to wait, if this PR is accepted. This could be some hours later or 4 weeks later. Until then I lost the interest or miss my ideas. If I have time, I like to do it in the flow. It's like on the road: you want to drive from A to B without a traffic jam. If you have to drive to C and wait for a undefined time and then to D and wait there again, driving wouldn't be the same.

@pauldendulk
Copy link
Member

@charlenni I understand you want to stay in the flow, but if that is your reason then I think you misunderstand how this kind of thing works in git/github. Elsewhere I proposed cumulative PRs, or a better term would be PR dependencies. Let me explain how that works.

You make some changes that would make a fine PR in themselves. You create a PR and now have to wait for the review. But you want to continue your work based on those changes. What you can do is create a new branch from the previous branch. Make new changes there. You can turn that into another PR and mention in the initial comment that the PR depends on the previous PR. In gitlab (not github) there is specific support for PR dependencies. In github you can just mention it in the comment. You could also introduce a third PR (but I guess too big a chain will not be handy).

Now in github you could review the first PR. You can merge it. Then In the second PR you can press the 'update branch' button in github. Now the commits from the first PR will disappear, and it can be reviewed.

In some cases modification in the first PR (because of the review) can cause conflicts in the second PR. You will have to resolve those. That can be cumbersome, but still better to do small step than having to handle everything in one big PR.

When I added this comment two weeks ago I was ready to merge this. The only thing I needed from you was to remove the draft (that is your responsibility). Without the draft I would have merged it right away. So part of your big PR would have been in main already, so quicker workflow. Also the diff of the second PR would be smaller and more readable, so easier to review, which would also result in quicker workflow. Big advantages, and there is no need for you to wait on anything.

If you do not really need PR dependencies (because the changes are not really related) then it is definitely preferred to keep those separate. So, no dependencies is preferred, but not always possible.

@charlenni
Copy link
Member Author

Ok @pauldendulk, I have the big PR also in 13 steps. Should I reopen this and you could merge it and then make a PR for each step? It is your decision.

@pauldendulk pauldendulk reopened this Jan 12, 2024
@pauldendulk pauldendulk merged commit c1f4d82 into Mapsui:main Jan 12, 2024
5 checks passed
@pauldendulk
Copy link
Member

pauldendulk commented Jan 12, 2024

To test the workflow I just re-opened this PR, merged it and updated my widget branch (made from your branch). It worked, but not fully as expected. The github update branch showed 10 conflicts, when reverting to a local git pull origin main there we no conflicts though.

No need to create PRs from separate commits. Small PRs is not just about small steps, but meaningful steps. There was a problem and the change is the solution, that is something I can follow. The 'why' is also important.

I resolved earlier conflicts in my branch, and will make a PR to your branch.

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.

None yet

2 participants