Skip to content

feat: add filter permission and group #393

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

Conversation

jlopes90
Copy link
Contributor

Supersedes #270

@datamweb datamweb added enhancement New feature or request tests needed Pull requests that need tests labels Aug 19, 2022
@kenjis
Copy link
Member

kenjis commented Aug 24, 2022

Note, we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. If pull requests are not accompanied by relevant tests, they will likely be closed.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#contributions

If you are not familiar with PHPUnit testing, See Resources

@jlopes90
Copy link
Contributor Author

@kenjis @datamweb

Sorry, I'm on vacation, I'll be back at the end of the month. So it's better to close the pull request. Otherwise I'll delay a lot of time doing tests (phpunit) or I can't.
So someone better make the new this PR and know how to do phpunit.

What you say?

@kenjis
Copy link
Member

kenjis commented Aug 24, 2022

No problem. If someone really wants this feature, they would take over this PR.
You don't need to close this.

@datamweb datamweb added the GPG-Signing needed Pull requests that need GPG-Signing label Sep 17, 2022
@sammyskills
Copy link
Contributor

No problem. If someone really wants this feature, they would take over this PR. You don't need to close this.

How does one take over or contribute to an existing PR?
I tried checking online but it seems I'll need to have write access to the repository.

Please advise.

@kenjis
Copy link
Member

kenjis commented Sep 29, 2022

Get this PR branch into your local repository, and push it to your GitHub repository.
And create your own PR.

If you have gh command:

$ gh pr checkout 393

You can get this add-filter-permission-and-group branch.

@MGatner
Copy link
Member

MGatner commented Sep 30, 2022

Or with just Git:

git remote add jlopes90 https://github.com/jlopes90/shield.git
git fetch jlopes90

You can then make a new branch starting from the existing one:

git switch -c my-takeover jlopes90/add-filter-permission-and-group

(I'm mobile so please excuse me if I didn't get something quiet right)

@datamweb datamweb added the stale Pull requests with conflicts label Sep 30, 2022
@datamweb
Copy link
Collaborator

datamweb commented Sep 30, 2022

In fact, I always wondered how @kenjis could handle PR created by others.
@MGatner Thanks! Now I learned completely.
Unfortunately, I received the following error from the mentioned method:

P:\MyGitHubWork\shield>git fetch jlopes90
fatal: unable to access 'https://github.com/jlopes90/shield.git/': OpenSSL SSL_connect: Connection was reset in connection to github.com:443

After a bit of research I found that setting SSH Key can fix the problem.
Now it works without any problem.
While troubleshooting I read the following:

  1. generating-a-new-ssh-key
  2. adding-a-new-ssh-key-to-your-account
  3. switching-remote-urls-from-https-to-ssh

I like this method, because other people's commits are not removed, and their contributions are recorded in the source. It is beautiful and fair.

P:\MyGitHubWork\shield>git fetch jlopes90
Enter passphrase for key '/c/Users/ppars/.ssh/id_ed50212':
remote: Enumerating objects: 68, done.
remote: Counting objects: 100% (37/37), done.
remote: Total 68 (delta 37), reused 37 (delta 37), pack-reused 31
Unpacking objects: 100% (68/68), 9.05 KiB | 52.00 KiB/s, done.
From github.com:jlopes90/shield
 * [new branch]      add-filter-permission-and-group -> jlopes90/add-filter-permission-and-group
 * [new branch]      develop    -> jlopes90/develop
 * [new branch]      docs       -> jlopes90/docs
 * [new branch]      magic-link-add-link-back-to-login -> jlopes90/magic-link-add-link-back-to-login
 * [new branch]      master     -> jlopes90/master
P:\MyGitHubWork\shield>git switch -c my-takeover jlopes90/add-filter-permission-and-group
Switched to a new branch 'my-takeover'
branch 'my-takeover' set up to track 'jlopes90/add-filter-permission-and-group'.

P:\MyGitHubWork\shield>git status
On branch my-takeover
Your branch is up to date with 'jlopes90/add-filter-permission-and-group'.

nothing to commit, working tree clean

@MGatner
Copy link
Member

MGatner commented Oct 1, 2022

Yay! Glad it was helpful. Working across remotes and preserving or co-authoring work is complicated but important, and once you do it a few times it starts to feel natural.

@kenjis kenjis added the good first issue Good for newcomers label Nov 4, 2022
@kenjis
Copy link
Member

kenjis commented Nov 4, 2022

This feature seems to be needed by many, so please feel free to send new PRs based on this PR if anyone can.

@lonnieezell
Copy link
Member

@MGatner @datamweb where do we stand on this?

@MGatner
Copy link
Member

MGatner commented Nov 15, 2022

I don't think anyone has claimed it.

@datamweb
Copy link
Collaborator

Comment 5: I think it has caused the audience to misunderstand.
I have just done a personal practice on Git there. I did not intend to use this PR.

So anyone can continue if they want.

@lonnieezell
Copy link
Member

Ok. I'll take this one over. It's a great start and a much needed feature.

@lonnieezell
Copy link
Member

Superseded by #535

@jlopes90 jlopes90 deleted the add-filter-permission-and-group branch May 17, 2023 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers GPG-Signing needed Pull requests that need GPG-Signing stale Pull requests with conflicts tests needed Pull requests that need tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants