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 Protocol::TRY #544

Merged
merged 3 commits into from
Jun 7, 2023
Merged

add Protocol::TRY #544

merged 3 commits into from
Jun 7, 2023

Conversation

ModProg
Copy link
Contributor

@ModProg ModProg commented Jun 5, 2023

  • added Protocol::TRY and call it in ?
  • add tests
  • bikeshed the name

@ModProg
Copy link
Contributor Author

ModProg commented Jun 5, 2023

What would the best place to add a test?

Should we keep TRY as the name?

I just used the hash calling .try() printed, not sure if that is what you
want.

@udoprog
Copy link
Collaborator

udoprog commented Jun 6, 2023

Naming seems perfect to me.

I just used the hash calling .try() printed, not sure if that is what you
want.

Commented elsewhere that they're randomly generated. I'm not exactly sure how you did the above, but if it's at risk of conflicting with something else in the future it's better to avoid!

cargo run --bin rune -- hash --random

@udoprog
Copy link
Collaborator

udoprog commented Jun 6, 2023

What would the best place to add a test?

Sorry, missed this. In the rune crate. I moved the tests in here so that they can make use of pub(crate) items and compile faster (linking is exceedingly slow on some platforms).

You can see a bunch of tests in crates/rune/src/tests. Just remember to add the module in tests.rs.

@ModProg
Copy link
Contributor Author

ModProg commented Jun 6, 2023

I'll add a test later, already tested it in my project and it works!

@ModProg ModProg requested a review from udoprog June 6, 2023 13:34
@udoprog udoprog merged commit 56ad8a5 into rune-rs:main Jun 7, 2023
@udoprog
Copy link
Collaborator

udoprog commented Jun 7, 2023

Cheers!

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.

2 participants