Skip to content

docs: add jwt demo#18

Closed
yance-dev wants to merge 1 commit intopycasbin:masterfrom
yance-dev:jwt_demo
Closed

docs: add jwt demo#18
yance-dev wants to merge 1 commit intopycasbin:masterfrom
yance-dev:jwt_demo

Conversation

@yance-dev
Copy link

@yance-dev yance-dev commented Apr 17, 2022

Fix: #14

@CLAassistant
Copy link

CLAassistant commented Apr 17, 2022

CLA assistant check
All committers have signed the CLA.

@casbin-bot
Copy link

@ffyuanda @Zxilly please review

@casbin-bot casbin-bot requested a review from ffyuanda April 17, 2022 16:23
@hsluoyz
Copy link
Member

hsluoyz commented Apr 18, 2022

@leeqvip @ffyuanda plz review

Copy link
Contributor

@Zxilly Zxilly left a comment

Choose a reason for hiding this comment

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

lgtm

@Zxilly
Copy link
Contributor

Zxilly commented Apr 18, 2022

plz change title to docs: add jwt demo, this should not trigger a new release

@yance-dev yance-dev changed the title feat: add jwt demo docs: add jwt demo Apr 18, 2022
@ffyuanda
Copy link
Member

I'm not familiar with this repo as @Zxilly does, so let's listen to him.

@ffyuanda
Copy link
Member

ffyuanda commented Apr 18, 2022

@hsluoyz @yance-dev @Zxilly

I lately realized that our collaboration process is somehow arbitrary.

A few aspects (not comprehensive enough, just examples):

  1. Commit messages are not well written. I suppose the commit message should
    provide more context/information/explanation behind each change. I understand
    that the 'docs: add jwt demo' is saying that this commit is "change docs, and add
    a jwt demo". But wouldn't this info be too terse and confusing for a stranger to this
    repo, just like me?

  2. The commit is way too ambiguous. The diff is suggesting that this commit does not only touch
    something as "docs", but also added more stuff like test suites and demo runs. I think
    things are going to be clearer if we can separate this mega-commit into a few commits,
    one thing at a time.

  3. Possibly PR "versioning"? I'm not so sure about this, but according to my previous
    contribution experience at PyCasbin, I found that we usually prefer this "rebase-and-force-push"
    strategy? I understand that you can use something like git reflog show <branch> to find the
    previous commit series, but is there any way that we can stop force-pushing, and show the
    "PR history" instead?

    As an example: if PR v1 has a typo, we all agreed that this typo should be
    fixed, then the contributor should git commit --amend or git rebase -i to fix this typo. And if
    not by force-push, is there a better way for us to see this typo fix across two different patch series?
    (Git itself comes with a git-range-diff to show the diff between two commit ranges, namely two patch
    series)

    Notice that this question comes totally from my personal idea, and it may be related to GitHub, so I'm not so
    sure if this is a good idea; and if it is a good idea, how can we do it?

Summary

These 3 points are certainly not comprehensive, and the spirit of this comment is to see
if we can bring some sort of coding guidelines to PyCasbin or even Casbin as a whole; I
believe something like "MyFirstContribution.md" or "SubmittingPatches.md" can be beneficial
to the organization in an all-rounded way.

I understand Casbin is a multilingual project, and such guidelines should not cover too much
language-specific conventions (it can contain some if necessary). But general contribution
processes, like how to write commits, how to organize your commits under a PR, should fall
into the discussion range.

P.S. I think this discussion can potentially belong to an Issue under Casbin repo, but I'm just
writing it here for convenience (within the context of this PR). If necessary, I can bring it up there.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 18, 2022

Our contibution docs is here: https://casbin.org/docs/en/contributing , welcome to polish it!

I'm OK with appending commits in the PR, but it doesn't mean so much as finally we will "Squash and merge" the PR into the trunk. So all commits will be finally be one anyway.

@ffyuanda
Copy link
Member

Our contibution docs is here: https://casbin.org/docs/en/contributing , welcome to polish it!

Sure.

I'm OK with appending commits in the PR, but it doesn't mean so much as finally we will "Squash and merge" the PR into the trunk. So all commits will be finally be one anyway.

Certainly. However, in case people want to see the git log of certain things, for example, see into
a merge commit using git log --oneline <commit>^-, I think a clearer commit history with more informative commit messages is more helpful.

@ffyuanda
Copy link
Member

  1. Possibly PR "versioning"? I'm not so sure about this, but according to my previous
    contribution experience at PyCasbin, I found that we usually prefer this "rebase-and-force-push"
    strategy? I understand that you can use something like git reflog show <branch> to find the
    previous commit series, but is there any way that we can stop force-pushing, and show the
    "PR history" instead?
    As an example: if PR v1 has a typo, we all agreed that this typo should be
    fixed, then the contributor should git commit --amend or git rebase -i to fix this typo. And if
    not by force-push, is there a better way for us to see this typo fix across two different patch series?
    (Git itself comes with a git-range-diff to show the diff between two commit ranges, namely two patch
    series)
    Notice that this question comes totally from my personal idea, and it may be related to GitHub, so I'm not so
    sure if this is a good idea; and if it is a good idea, how can we do it?

Nevermind this one, I think GitHub provides a way to see the diff between two commit ranges
if you force-pushed. I guess we should write something about this in the https://casbin.org/docs/en/contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants