Skip to content

Rename crate::exid::ExId to crate::obj_id::ObjId #755

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

Closed
wants to merge 1 commit into from

Conversation

teohhanhui
Copy link
Contributor

No description provided.

@teohhanhui teohhanhui mentioned this pull request Oct 4, 2023
@alexjg
Copy link
Collaborator

alexjg commented Oct 4, 2023

Now, the interesting question is whether this has any compatibility effects. It seems like it should definitely not, because the exported name and type have not changed, but I am cautious. @teohhanhui did you encounter anywhere when you wrote code against this library that had to use ExId rather than ObjId?

@teohhanhui
Copy link
Contributor Author

Now, the interesting question is whether this has any compatibility effects. It seems like it should definitely not, because the exported name and type have not changed, but I am cautious.

If we used type alias it'd have compatibility concerns: https://predr.ag/blog/re-exporting-enum-with-type-alias-breaking-change-not-major/

But we didn't. There are no compatibility concerns that I'm aware of.

@teohhanhui did you encounter anywhere when you wrote code against this library that had to use ExId rather than ObjId?

No.

@teohhanhui
Copy link
Contributor Author

This change doesn't fix everything. Wherever ExId is still present in impl block header or function / method signature...

@alexjg
Copy link
Collaborator

alexjg commented Oct 4, 2023

This change doesn't fix everything. Wherever ExId is still present in impl block header or function / method signature...

Can you elaborate?

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Oct 4, 2023

e.g.

pub fn hash_for_opid(&self, opid: &ExId) -> Option<ChangeHash> {

results in

Screenshot 2023-10-04 at 23-51-40 AutoCommit in automerge - Rust

but it links correctly to automerge::ObjId (same as before).

So what this PR achieves is pretty minimal. I've limited myself to only getting rid of any mention of ExId from the automerge::ObjId page itself, and the one case of [`ExId`] in the rustdoc / doc comment that I could find.

@teohhanhui
Copy link
Contributor Author

Looks like that will be fixed soon: rust-lang/rust#113374

@alexjg
Copy link
Collaborator

alexjg commented Oct 4, 2023

Hmm, do we need to make this change then?

@teohhanhui
Copy link
Contributor Author

In the linked PR, I don't see any tests for impl block header.

@teohhanhui
Copy link
Contributor Author

Looks like that's rust-lang/rust#41072

@alexjg
Copy link
Collaborator

alexjg commented Oct 4, 2023

I'm inclined to think that this is a rustdoc bug and we should just wait for it to be fixed as even with these changes we end up with ExId leaking out, wdyt?

@teohhanhui teohhanhui closed this Oct 4, 2023
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