Skip to content

diff-containers: Remove Unsafe* versions of DiffEntry constructors #11

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 1, 2023
Merged

diff-containers: Remove Unsafe* versions of DiffEntry constructors #11

merged 1 commit into from
Feb 1, 2023

Conversation

@jorisdral
Copy link
Contributor

@jorisdral jorisdral commented Jan 31, 2023

New insights show that the Unsafe* constructors are not necessary for the Group instances that we define.

@jorisdral jorisdral self-assigned this Jan 31, 2023
@jorisdral jorisdral added the enhancement New feature or request label Jan 31, 2023
@jorisdral jorisdral force-pushed the jdral/diff-containers-remove-unsafe-diff-entries branch from c23da5a to 25889bb Compare January 31, 2023 15:37
@jorisdral jorisdral mentioned this pull request Jan 31, 2023
Closed
7 tasks
@jasagredo
Copy link
Contributor

jasagredo commented Feb 1, 2023

I think the commits got messed up? I see that the 2nd commit on this branch is the only one on #10 , so probably you are missing a rebase somewhere?

jasagredo
jasagredo approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

Looks good. I think these would be breaking changes if you had released in CHaP but I don't think you have releases these yet, right?

@jorisdral jorisdral mentioned this pull request Feb 1, 2023
Merged
@jorisdral
Copy link
Contributor Author

jorisdral commented Feb 1, 2023

I think the commits got messed up? I see that the 2nd commit on this branch is the only one on #10 , so probably you are missing a rebase somewhere?

You're right, I did not rebase this PR and #12 properly yet. Before I merge the PR, I will make sure to perform a rebase for a nice commit history

Looks good. I think these would be breaking changes if you had released in CHaP but I don't think you have releases these yet, right?

Yes, they would have been breaking changes, but the package is not released to CHaP yet. I am also ensuring that ouroboros-consensus builds correctly before releasing the first version to CHaP.

@jorisdral jorisdral marked this pull request as ready for review February 1, 2023 12:46
@jorisdral
Remove Unsafe* constructors for the DiffEntry type.
bb02a53 
New insights show that the `Unsafe*` constructors are not necessary
for the `Group` instances that we define.
@jorisdral jorisdral force-pushed the jdral/diff-containers-remove-unsafe-diff-entries branch from 25889bb to bb02a53 Compare February 1, 2023 12:55
@jorisdral jorisdral merged commit e5c3d21 into main Feb 1, 2023
@jorisdral jorisdral deleted the jdral/diff-containers-remove-unsafe-diff-entries branch February 1, 2023 13:07
@jorisdral jorisdral mentioned this pull request Feb 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnadales dnadales dnadales approved these changes

@jasagredo jasagredo jasagredo approved these changes

Assignees

@jorisdral jorisdral

Labels

enhancement New feature or request

Projects

No open projects
Consensus Team Backlog
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisdral @jasagredo @dnadales