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

Batchrunner: Remove unnecessary dict transformation, .keys() in len() #1460

Merged
merged 1 commit into from
Oct 30, 2022

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Oct 20, 2022

After inspiration from #1456, I checked the rest of the codebase on such cases, and found two in the Batchrunner

  • Removes one unnecessary dict transformation
  • Removes three unnecessary calls of .keys() when getting the dict length

@EwoutH EwoutH changed the title Batchrunner: Remove unnecessary dict transformation, keys() in len() Batchrunner: Remove unnecessary dict transformation, .keys() in len() Oct 20, 2022
@codecov
Copy link

codecov bot commented Oct 29, 2022

Codecov Report

Base: 91.15% // Head: 91.34% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (9000e8c) compared to base (af23b8b).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 9000e8c differs from pull request most recent head 6e60a83. Consider uploading reports for the commit 6e60a83 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1460      +/-   ##
==========================================
+ Coverage   91.15%   91.34%   +0.18%     
==========================================
  Files          15       15              
  Lines        1289     1305      +16     
  Branches      218      223       +5     
==========================================
+ Hits         1175     1192      +17     
+ Misses         81       80       -1     
  Partials       33       33              
Impacted Files Coverage Δ
mesa/batchrunner.py 92.88% <100.00%> (-0.04%) ⬇️
mesa/time.py 94.05% <0.00%> (-0.74%) ⬇️
mesa/datacollection.py 97.67% <0.00%> (-0.03%) ⬇️
mesa/visualization/ModularVisualization.py 78.88% <0.00%> (+0.13%) ⬆️
mesa/space.py 96.23% <0.00%> (+0.68%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@EwoutH
Copy link
Member Author

EwoutH commented Oct 29, 2022

@rht Thanks for the review, fixed the dict call, CI now passes!

If you want me to clean up this PR to a single commit please let me know, but I'm good with you squashing during merge.

@rht
Copy link
Contributor

rht commented Oct 30, 2022

The squash and merge GH button is disabled, so I can't squash it. Using this button will create a merge commit in main, so it is intentionally disabled.

@EwoutH
Copy link
Member Author

EwoutH commented Oct 30, 2022

Using this button will create a merge commit in main, so it is intentionally disabled.

It doesn’t though, I use it on most of my repos. I describe the process we use on one of my professors repos here, to keep an atomic commit history without any merge commits. Also the GitHub docs provide some insight.

- Removes one unnecessary dict transformation
- Removes three unnecessary calls of .keys() when getting the dict lenght
@EwoutH
Copy link
Member Author

EwoutH commented Oct 30, 2022

But for now, I squashed them to a single commit :)

@EwoutH EwoutH force-pushed the batch-dict-trans-len-keys branch from 9000e8c to 6e60a83 Compare October 30, 2022 08:26
@rht
Copy link
Contributor

rht commented Oct 30, 2022

Using this button will create a merge commit in main, so it is intentionally disabled.

It doesn’t though, I use it on most of my repos. I describe the process we use on one of my professors repos here, to keep an atomic commit history without any merge commits. Also the GitHub docs provide some insight.

I wanted to test on github.com/rht/mesa, but Accidentally made a PR to github.com/projectmesa/mesa that got merged. But anyway, I confirm that it works, and it has been enabled now.

@rht rht merged commit b7dd15e into projectmesa:main Oct 30, 2022
@EwoutH
Copy link
Member Author

EwoutH commented Oct 30, 2022

Thanks for merging, and for testing and validating!

One thing to keep in mind is that sometimes you have to clean up the commit message a bit, but aside from that, it’s a really great tool in the toolbox.

@jackiekazil jackiekazil added this to the v1.2.0 Taylor milestone Feb 27, 2023
@jackiekazil jackiekazil mentioned this pull request Mar 7, 2023
4 tasks
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.

3 participants