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 deleteReference #388

Closed
wants to merge 2 commits into from
Closed

Add deleteReference #388

wants to merge 2 commits into from

Conversation

pbrisbin
Copy link
Contributor

NOTE: I'm not sure if passing a Name GitReference is right, or if I should pass a full GitReference.

For example, deleting a branch works with this code: deleteReference auth owner repo "heads/some-branch". If I were required to pass a full GitReference, I'd have to either fake one or somehow have fetched it via a prior API call. This limitation is why I did what I did here, FWIW.

@phadej
Copy link
Contributor

phadej commented Jun 18, 2019

There is no Name GitReference in the lib. Either use Text, because gitReferenceRef :: Text, or change the type of latter.

Copy link
Contributor

@phadej phadej left a comment

Choose a reason for hiding this comment

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

As commented previously

@pbrisbin
Copy link
Contributor Author

Gotcha, I'll just take Text, I think.

I might've misunderstood the philosophy behind Name. I treated it as a tagged Text to avoid errors (e.g. transposition) and didn't think it mattered what you were naming (in this case a GitReference).

Is there any convention for what can or cannot be named? This code does function FWIW, even though there "is no Name GitReference in the lib", which I'm not sure I totally understand.

@phadej
Copy link
Contributor

phadej commented Jun 18, 2019

@pbrisbin yes. I'd prefer change of gitReferenceRef field. So if you have a collection of GitReferences, you can remove one of them by looking up the name from there.

See e.g. https://hackage.haskell.org/package/github-0.22/docs/GitHub-Data-GitData.html#t:GitCommit, there gitCommitSha is a Name.

@pbrisbin
Copy link
Contributor Author

OK, if I'm understanding correctly: you don't want to just toss around Name X as a tagged Text, you prefer to introduce them by having a field on X that represents its name, then that can be a Name X, and then you can use it elsewhere. 👍

Do you have any concerns about package API compatibility? I think changing the type of gitReferenceRef would require a major bump vs either this PR as-is or passing a Text, which would be minor. I'll go ahead and do it, just wanted to raise the point.

Also, total sidenote: there can exist a GitCommit without sha!? I just noticed in your link that field is Maybe. That's crazy.

@phadej
Copy link
Contributor

phadej commented Jun 19, 2019

breaking changes is fine

@pbrisbin
Copy link
Contributor Author

pbrisbin commented Jun 19, 2019

This is ready for re-review.

@pbrisbin
Copy link
Contributor Author

pbrisbin commented Sep 3, 2019

👋 Hi there, I just wanted to check that you're not waiting on me for anything here?

@phadej phadej self-requested a review November 27, 2019 12:32
@phadej
Copy link
Contributor

phadej commented Nov 27, 2019

I missed that for github-0.24. Will merge soon-ish.

@pbrisbin
Copy link
Contributor Author

Did this get missed for github-0.25 too?

@phadej
Copy link
Contributor

phadej commented May 26, 2020

amended and merged as part of #446, thanks! Sorry for long wait.

@phadej phadej closed this May 26, 2020
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