NIP-34: add PR and PR update events #1966
Conversation
|
Looks good to me. |
fd6bc27 to
e9073a5
Compare
|
LGTM, I like the idea that I can create a PR with and without GRASP server. Make it more flexible. |
e9073a5 to
fadcf2a
Compare
|
this last update makes the |
fadcf2a to
b0d94e3
Compare
|
last update adds line: Stirfry max event size default is 65kb and nostr-rs-relay is 128kb. |
b0d94e3 to
6a7ac11
Compare
|
last update handles what SHOULD happen when an additional patch would exceed the 60kb limit. It enables a PR as a patch-revision. |
|
Perhaps we should think about applying nip-22 style threading to Status and Patches as propsed in #1799 ? |
|
@dluvian @Silberengel do you have any thoughts about all this? |
| ["p", "<repository-owner>"], | ||
| ["p", "<other-user>"], // optionally send the PR to another user to bring it to their attention |
There was a problem hiding this comment.
Uppercase P for repo-owner and lowecase p for people directly mentioned in the PR. Repo owner could be in P and p if he was mentioned directly in it. Gives client devs the option to improve notification/inbox filtering
There was a problem hiding this comment.
That makes a lot of sense and provides more flexibility. However, an uppercase P is used only for the author of the "root scope event" in NIP-22. Its use for other purposes isn't prohibited in NIP-22, but I haven't seen it used anywhere else. Will we break non-git-specialist clients by doing this? For example, clients may:
- Assume there is only one
Ptag. - Assume it can't be the thread root but get confused as there is no corresponding
Etag. - Carry these
Ptags through to replies.
There was a problem hiding this comment.
Isn't a repo the root space of a PR?
There was a problem hiding this comment.
Isn't a repo the root space of a PR?
In the context of a PR, Patch or Issue, its important that repo announcement isn't mistaken for the root of the thread. Otherwise a single filter can't be used to find all comments related to that thread.
There was a problem hiding this comment.
In the context of a PR, Patch or Issue, its important that repo announcement isn't mistaken for the root of the thread.
Makes sense.
There was a problem hiding this comment.
Your suggestion is a really good idea but we need a bit more thought around the above question before adding it to the spec:
Will we break non-git-specialist clients by doing this?
| ["p", "<repository-owner>"], | ||
| ["p", "<other-user>"], // optionally send the PR to another user to bring it to their attention |
6a7ac11 to
7468655
Compare
|
We never wanted patches. |
d28f8c3 to
5599fc4
Compare
|
@Silberengel wrote in this comment
So why do we use GitViaNostr if it's not for decentralized patches and issues? |
|
@dluvian wrote in this comment
Is this because of the PR revision of patches? Then what do you think about closing the patch and open a PR if the revision > 60KB? |
You aren't even using it. You're using GitHub. We tried to use ngit patches, as a team, and the repo quickly turned into 🍝. So, nope. |
You didn't answer my question, anyway, I'm using patches in Nostr and it's good, you just make the patches and send them or fetch them and apply them, I don't know why your repo turned into a spaghetti |
|
Because the commits were too large. |
That's still more work than simply ignoring patches and only implementing PRs. I do prefer patches and their simplicity but if their size restriction is a practical issue I would ignore them and only work with PRs |
|
@cypherhoodlum are you still working on a git stuff client? Any thoughts on this? |
Does it make sense to write in the repo announcement that you only accept patches or only PRs or both? Then whatever client/relay the repository owner is choosing can keep that field updated? Patches are great because they don't require a server, and we cannot guarantee that everybody will have access to a server, even if it's commoditized with GRASP. Servers may still require a setup or registration process in the long run, and for some small things just issuing a "send patch" command from the command line is probably better. @DanConwayDev what do you think about vastly simplifying the patch flow to make it only use one event, throwaway, very simple and dumb -- and leave any slightly more complicated flow to PRs? |
My concern is that repository maintainers won't make a considered or informed choice; instead, a client with an ideological bent for one or the other will choose for them, without the maintainer understanding the implications. imo, this decision shouldn't be made during onboarding, as it assumes too much knowledge and requires too much thought, which would degrade the user experience.
Most of the complexity is optional and degrades gracefully. You would still need |
Also make a set of patches in one single event will make the patches useless, because they will not be accepted by most of the relays because their events are large. I think the current approach are good, use patches if each commit is less than 60KB otherwise use PRs. |
|
For me, I prefer the patches, and I hope developers don't use PRs as a replaceable for the patches, because patches are more decentralized than PRs. PRs for the large changes, and don't make large changes frequently, you should maintain your change size and focus on one topic instead of multiple. I mean if you reach 60 KB for one single patch you are not doing well. |
5599fc4 to
1eea2d4
Compare
|
this last updated improves the wording around populating the clone tag and the git servers push requirements, including how to identify grasp servers from the announcement event. |
1eea2d4 to
3f9d86a
Compare
|
this last update adds tags to |
With the advent of Grasp, we can now do PRs without relying on a single contributor-selected Git server, which they would have to provision and could be rugged at any time. Note how the commit ID is included in the event and any updates, be it force pushes or additional commits, are added by supplementary PR update events. This ensures history is maintained and Nostr events remain the authority, rather than Git servers. Grasp servers with proactive sync enabled will copy the commits to ensure that they are available if the contributor-selected Git server is unreliable or removed. This also provides an alternative for large patches, which are too large to be included within a signle nostr event. It is also more supported by Git tools which don't always have patch support. For example, isomorphic-git, which enables Git in the browser.
3f9d86a to
6ee3532
Compare
|
this last update simplies the Patch ~> PR path. I expect this very much to be an edge case. |
With the advent of Grasp, we can now do PRs without relying on a single contributor-selected Git server, which they would have to provision and could be rugged at any time.
Note how the commit ID is included in the event and any updates, be it force pushes or additional commits, are added by supplementary PR update events. This ensures history is maintained and Nostr events remain the authority, rather than Git servers.
Grasp servers with proactive sync enabled will copy the commits to ensure that they are available if the contributor-selected Git server is unreliable or removed.
This also provides an alternative for large patches, which are too large to be included within a signle nostr event.
It is also more supported by Git tools which don't always have patch support. For example, isomorphic-git, which enables Git in the browser.