Skip to content

Conversation

@Lioness100
Copy link
Contributor

Fixed one or two typos 😉 (this was a trial run for a CLI I made)


Ok for Byron review the PR on video?

  • I give my permission to record review and upload on YouTube publicly

If I think the review will be helpful for the community, then I might record and publish a video.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot, it's a pretty decent result and a lot of mistakes removed. Much appreciated.

I noticed that it doesn't shy away from fixing spelling mistakes in enumeration variants, which technically is a breaking change. All of these changes where in errors so I think it's OK as people are unlikely to actually match on these cases.

On another note, it was happy to change unencoded to encoded, inverting the meaning in the process. I will change that back.

);

assert_eq!(config.string("doesnt", None, "exist"), None);
assert_eq!(config.string("doesn't", None, "exist"), None);
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting as I didn't dare putting ' into section names for query. But it's OK to do that and has the same effect, good to see.

fn contains(&self, id: impl AsRef<git_hash::oid>) -> bool;

/// Find an object matching `id` in the database while placing its raw, undecoded data into `buffer`.
/// Find an object matching `id` in the database while placing its raw, encoded data into `buffer`.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't correct - it should remain unencoded or something semantically similar, not opposite.

Copy link
Contributor Author

@Lioness100 Lioness100 Feb 8, 2023

Choose a reason for hiding this comment

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

Hi, sorry about that, I think the confusion came from that it's originally "undecoded", not "unencoded". Either way, since both undecoded and unencoded aren't actually words, I think it would be better to use their positive counterpoint, encoded and decided respectively.

@Byron Byron merged commit cd84c1a into GitoxideLabs:main Feb 8, 2023
@Lioness100
Copy link
Contributor Author

My pleasure!

@Lioness100
Copy link
Contributor Author

@Byron, did you see my response to your comment?

Byron added a commit that referenced this pull request Feb 9, 2023
Thank you @Lioness100 for the persistence :).
@Byron
Copy link
Member

Byron commented Feb 9, 2023

My apologies - I didn't realise my mistake until now. Thank you for the persistence (it's fixed now)!

@Lioness100
Copy link
Contributor Author

No problem, glad I could help!

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