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

remove deprecated skip_equivalent from pyproj #43

Closed

Conversation

wang-boyu
Copy link
Member

Fix #39 due to pyproj4/pyproj#824

@@ -96,7 +94,7 @@ def get_intersecting_agents(self, agent, other_agents=None):
return intersecting_agents

def get_neighbors_within_distance(
self, agent, distance, center=False, relation="intersects"
self, agent, distance, center=False, relation="intersects"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an unnecessary indentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for spotting this! It's probably due to the auto formating tool from my IDE.

@rht
Copy link
Contributor

rht commented Apr 2, 2022

Merged as 75d356e.
I was about to ask you to squash your commits, but figured it'd be faster if I squash it myself.

@rht rht closed this Apr 2, 2022
@rht
Copy link
Contributor

rht commented Apr 2, 2022

Hmmm, while it is common for some FOSS projects to merge it the way I just did, 75d356e doesn't link to this PR. This way, generating the changelog items will be harder, where we usually link to the relevant PRs. I'm not going to do this in the future.

@wang-boyu
Copy link
Member Author

Hmmm, while it is common for some FOSS projects to merge it the way I just did, 75d356e doesn't link to this PR. This way, generating the changelog items will be harder, where we usually link to the relevant PRs. I'm not going to do this in the future.

This is interesting to know - all my previous projects (proprietary software) were using the merge method with --no-ff, but I'm fine with alternative ways.

In the future for mesa and mesa-geo, would you suggest to squash all commits before submitting a PR?

If yes then perhaps we can also standardise and mention it in the CONTRIBUTING.rst.

@wang-boyu wang-boyu deleted the fix/deprecate-skip_equivalent branch April 2, 2022 02:10
@rht
Copy link
Contributor

rht commented Apr 2, 2022

Squashing all commits is problematic, see e.g. JuliaDynamics/Agents.jl@b1cd6a8, which is a giant commit where the intent of each individual change is hard to read. I favor atomic commits that each logically do different things. Which means, small changes with nonsensical commit message should be squashed into the closest commit that does the same thing. It is a bit too much to expect every contributor to do this fine-grained squashing, but then again, it is super rare for a contributor to create a large PR in the first place.

@rht
Copy link
Contributor

rht commented Apr 2, 2022

Though you could argue that in JuliaDynamics/Agents.jl@b1cd6a8, you can still see individual commits in the pull requests.

@wang-boyu
Copy link
Member Author

Yeah I agree that squashing is problematic, guess this is why I seldom use it over the years.

On the other hand, if individual commits are preserved and the rebase method is used for pull requests, then the history of the main branch will be full of these small commits from different branches intertwined. This is basically why we (my previous projects) preferred the merge method with --no-ff for PRs. But again, I'm fine with any protocol so long as it's mutually agreed.

@rht
Copy link
Contributor

rht commented Apr 5, 2022

I just remembered one reason keeping the commits separate instead of squashed for each PR is worthwhile: doing git-bisect to find a commit that causes a bug is much easier, vs narrowing it down to 1 giant commit.

@wang-boyu
Copy link
Member Author

Yup!

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.

DeprecationWarning: skip_equivalent is deprecated with mesa-geo 0.2.0 and pyproj 3.3.0
2 participants