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

fix: satisfy conditions for semver-check #323

Merged

Conversation

ghost
Copy link

@ghost ghost commented Oct 6, 2023

No description provided.

@ghost ghost changed the title fix: satitify conditions for semver-check fix: satisfy conditions for semver-check Oct 6, 2023
@ghost ghost mentioned this pull request Oct 6, 2023
@ghost ghost changed the base branch from main to release-plz/2023-10-05T19-56-47Z October 6, 2023 15:46
@frol
Copy link
Collaborator

frol commented Oct 10, 2023

@shariffdev I am not familiar with UnwindSafe, so I have several questions in my head:

  1. Why did we lose auto-trait implementation there in the first place?
  2. Is it safe to just impl explicitly?
  3. Do we need to impl it explicitly?

Could you dig into it?

@ghost
Copy link
Author

ghost commented Oct 10, 2023

Sure, looking into it.

@ghost
Copy link
Author

ghost commented Oct 10, 2023

Why did we lose auto-trait implementation there in the first place?
Do we need to impl it explicitly?

The GasHook introduced did not implement the auto-traits because it does not fall under the rules the compiler uses for auto implementation. We had to add Sync and Send explicitly which were also not auto-implemented. Reference: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits

Is it safe to just impl explicitly?

The traits behave similarly in implementation to Send and Sync which we have also explicitly implemented. Reference: https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

Thanks for diving into it! This looks reasonable now

@frol frol merged commit 1a2bc0f into near:release-plz/2023-10-05T19-56-47Z Oct 10, 2023
6 checks passed
@frol
Copy link
Collaborator

frol commented Oct 10, 2023

Oh, it should have targeted main branch, and then release-plz would pick it up from there.

@ghost
Copy link
Author

ghost commented Oct 11, 2023

I missed that! Is anything to be done or we leave it?

frol pushed a commit that referenced this pull request Oct 13, 2023
frol added a commit that referenced this pull request Oct 13, 2023
…to avoid breaking release (#323) (#326)

Co-authored-by: Yasir Shariff <141004977+shariffdev@users.noreply.github.com>
@frol frol mentioned this pull request Oct 22, 2023
@ghost ghost deleted the fix/semver-check branch November 9, 2023 13:26
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.

1 participant