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

Add utility method to format dictionary #65

Merged
merged 5 commits into from
Oct 20, 2023

Conversation

DhruvDuseja
Copy link
Contributor

@DhruvDuseja DhruvDuseja commented Oct 19, 2023

Summary

Major changes:

Todos

If this is work in progress, what else needs to be done?

  • Need help with the documentation

Checklist

  • Google format doc strings added.
  • Code linted with ruff. (For guidance in fixing rule violates, see rule list)
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • I have run the tests locally and they passed.

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
src/pyEQL/utils.py 100.00% <100.00%> (ø)

📢 Thoughts on this report? Let us know!.

Copy link
Member

@rkingsbury rkingsbury left a comment

Choose a reason for hiding this comment

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

Thank you @DhruvDuseja ! It looks like tests are passing so this is close to ready. I have two minor changes to request (see comments), and then feel free to remove WIP status and I think it'll be good to merge.

src/pyEQL/utils.py Outdated Show resolved Hide resolved
src/pyEQL/utils.py Outdated Show resolved Hide resolved
@DhruvDuseja
Copy link
Contributor Author

DhruvDuseja commented Oct 19, 2023

Thanks for the feedback @rkingsbury. I've made the changes in the doc and added the test. Please let me know if this looks good. Hopefully, the doc reads better now but please let me know if there are any changes needed there.

rkingsbury
rkingsbury previously approved these changes Oct 20, 2023
Copy link
Member

@rkingsbury rkingsbury left a comment

Choose a reason for hiding this comment

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

Looks great! I made some minor comments on the docstring. The last thing I request is that you please update the changelog. This will be the first change after the last release, so you can add a new section (following the Keep a Changelog format) like this:

## [Unreleased]

### Added
-

Please take out of WIP status when you're ready, and I'll be happy to merge.

src/pyEQL/utils.py Outdated Show resolved Hide resolved
src/pyEQL/utils.py Outdated Show resolved Hide resolved
src/pyEQL/utils.py Outdated Show resolved Hide resolved
@DhruvDuseja
Copy link
Contributor Author

Thanks @rkingsbury. I've made the required changes, hope this looks good!

@DhruvDuseja DhruvDuseja marked this pull request as ready for review October 20, 2023 05:07
@rkingsbury rkingsbury added the feature Adds new feature label Oct 20, 2023
@rkingsbury
Copy link
Member

Closes #65

Thanks @DhruvDuseja !

@rkingsbury rkingsbury merged commit 796ca2f into KingsburyLab:main Oct 20, 2023
13 checks passed
@DhruvDuseja DhruvDuseja deleted the format_solute_utility branch October 20, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add utility function to format dict with single unit
2 participants