Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

feat: provide Into<String> for identifier types #974

Merged
merged 3 commits into from
Nov 22, 2023
Merged

feat: provide Into<String> for identifier types #974

merged 3 commits into from
Nov 22, 2023
+15 −4

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Nov 20, 2023 •
edited
Loading

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@codecov Codecov
Copy link

codecov bot commented Nov 20, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (71df7e8) 70.24% compared to head (17d0fb0) 70.23%.
Report is 1 commits behind head on main.

Files Patch % Lines
...-core/ics24-host/types/src/identifiers/chain_id.rs 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #974      +/-   ##
==========================================
- Coverage   70.24%   70.23%   -0.02%     
==========================================
  Files         177      177              
  Lines       17730    17733       +3     
==========================================
  Hits        12455    12455              
- Misses       5275     5278       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Farhad-Shabani
Farhad-Shabani approved these changes Nov 22, 2023
View reviewed changes
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

Thanks @mina86 🌷
Just regarding the AsRef<str> we already have as_str methods for all the identifiers. I assume that should work for you.

@Farhad-Shabani Farhad-Shabani changed the title feat: provide Into<String> and AsRef<str> for identifier types feat: provide Into<String> for identifier types Nov 22, 2023
@Farhad-Shabani Farhad-Shabani merged commit 45b1c97 into informalsystems:main Nov 22, 2023
@mina86 mina86 deleted the f branch November 22, 2023 18:10
@mina86
Copy link
Contributor Author

mina86 commented Nov 22, 2023

@Farhad-Shabani, for sure. I actually don’t care about AsRef<str> at all. I've only added it for consistency. Notice that ChannelId and PortId do implement AsRef<str> so IMO it makes sense for all of the identifiers to implement it (or alternatively for none of them to implement it).

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Nov 22, 2023 •
edited
Loading

Ahh, that's right. Actually, either one is redundant!, feel free to open another PR. Perhaps keeping AsRef<str> ?

Farhad-Shabani added a commit that referenced this pull request Sep 9, 2024
@mina86 @Farhad-Shabani
feat: provide Into<String> for identifier types (#974)
57291ef 
* feat: provide `Into<String>` and `AsRef<str>` for identifier types

* fix: use as_str on chain_id

---------

Co-authored-by: Farhad Shabani <farhad.shabani@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Farhad-Shabani Farhad-Shabani Farhad-Shabani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mina86 @Farhad-Shabani