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

Feat/submit #271

Merged
merged 55 commits into from
Aug 20, 2024
Merged

Feat/submit #271

merged 55 commits into from
Aug 20, 2024

Conversation

2byrds
Copy link
Collaborator

@2byrds 2byrds commented Jul 18, 2024

Adds a KERIA endpoint /identifiers/${name}/submit to resubmit an identifier KEL to be receipted by it's witnesses.
Adds 'submit' unit tests
This feature is the KERIA equivalent to the keripy witness 'submit' command line tool

2byrds added 30 commits June 3, 2024 08:21
…sed credentialing design for IdentifierResource end usage

Signed-off-by: 2byrds <[email protected]>
Signed-off-by: 2byrds <[email protected]>
Copy link
Collaborator Author

@2byrds 2byrds left a comment

Choose a reason for hiding this comment

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

self-review in order to 'mark' the changes in the midst of many formatting changes.

src/keria/app/agenting.py Show resolved Hide resolved
src/keria/app/agenting.py Outdated Show resolved Hide resolved
src/keria/app/agenting.py Outdated Show resolved Hide resolved
src/keria/app/agenting.py Outdated Show resolved Hide resolved
src/keria/app/aiding.py Show resolved Hide resolved
tests/app/test_aiding.py Show resolved Hide resolved
tests/app/test_aiding.py Outdated Show resolved Hide resolved
tests/app/test_aiding.py Outdated Show resolved Hide resolved
tests/app/test_specing.py Show resolved Hide resolved
tests/app/test_specing.py Outdated Show resolved Hide resolved
Signed-off-by: 2byrds <[email protected]>
Signed-off-by: 2byrds <[email protected]>
@2byrds 2byrds marked this pull request as ready for review July 18, 2024 21:07
@2byrds 2byrds requested a review from pfeairheller July 18, 2024 21:08
Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

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

This PR will not be accepted with formatting changes, and as a general open source software contribution practice formatting changes should never be included in PRs that address features or bug fixes. In addition formatting changes that cause files to differ from each other in the same repo should not be submitted.

Please resubmit without the mass reformatting of files so we can review the actual changes.

@pfeairheller
Copy link
Member

self-review in order to 'mark' the changes in the midst of many formatting changes.

Being perfectly frank here, this stands out as a red flag. It would be irresponsible as maintainers of a security focused open source project to accept a PR with this many changes that the submitter is asking us to ignore. And the changes are not appropriate for the bug being fixed and the amount of reformatting makes it very hard to determine what was actually changed.

PRs should be laser focused on what they are fixing. If we want to reformat the Python, that should be discussed, an issue open and a systematic approach taken.

@2byrds
Copy link
Collaborator Author

2byrds commented Jul 18, 2024

@pfeairheller do you know what formatter you use so I can change my auto-formatter. It's a useful IDE tool so if I can align with you that would be ideal.

@2byrds
Copy link
Collaborator Author

2byrds commented Jul 19, 2024

@pfeairheller signify-ts has a github action that checks formatting for all PRs. are you open to a similar github action in KERIA so that formatting is aligned and enforced?

@2byrds
Copy link
Collaborator Author

2byrds commented Jul 19, 2024

self-review in order to 'mark' the changes in the midst of many formatting changes.

Being perfectly frank here, this stands out as a red flag. It would be irresponsible as maintainers of a security focused open source project to accept a PR with this many changes that the submitter is asking us to ignore. And the changes are not appropriate for the bug being fixed and the amount of reformatting makes it very hard to determine what was actually changed.

PRs should be laser focused on what they are fixing. If we want to reformat the Python, that should be discussed, an issue open and a systematic approach taken.

Great!
I don't know what you mean by the changes are not appropriate for the bug being fixed. It is introducing a feature that is similar to the keripy feature.
As for formatting i'll create a test PR that standardizes the format and introduces a format check. If you would like to be the one to submit the reformatted repo so that it is attributed to a maintainer that is cool with me.
Then we'll be well formatted, aligned, and all future PRs enforced.

@2byrds
Copy link
Collaborator Author

2byrds commented Jul 19, 2024

@pfeairheller here is a draft PR to add the github action for checking python via 'black'.
#273
I have no preference on the formatter we use, this is a common one.
You can see the diagnostic output of the check here https://github.com/WebOfTrust/keria/pull/273/checks
perhaps you will want to submit the PR as a maintainer with the reformatted repo.

pip install black

black --check .

black .

and once the formatting is aligned i will update this PR to align (all PRs will be forced to align from now on).

@pfeairheller
Copy link
Member

@pfeairheller do you know what formatter you use so I can change my auto-formatter. It's a useful IDE tool so if I can align with you that would be ideal.

I use IntelliJ's support for the PEP formatting rules.

@pfeairheller
Copy link
Member

@pfeairheller here is a draft PR to add the github action for checking python via 'black'. #273 I have no preference on the formatter we use, this is a common one. You can see the diagnostic output of the check here https://github.com/WebOfTrust/keria/pull/273/checks perhaps you will want to submit the PR as a maintainer with the reformatted repo.

pip install black

black --check .

black .

and once the formatting is aligned i will update this PR to align (all PRs will be forced to align from now on).

I believe the signify-ts only does format checking, not reformatting. Something I've run afoul of, having to push changes to PRs because they failed the check. I'd like to take that approach here and not one that changes the code automatically.

@2byrds
Copy link
Collaborator Author

2byrds commented Jul 19, 2024

@pfeairheller here is a draft PR to add the github action for checking python via 'black'. #273 I have no preference on the formatter we use, this is a common one. You can see the diagnostic output of the check here https://github.com/WebOfTrust/keria/pull/273/checks perhaps you will want to submit the PR as a maintainer with the reformatted repo.

pip install black

black --check .

black .

and once the formatting is aligned i will update this PR to align (all PRs will be forced to align from now on).

I believe the signify-ts only does format checking, not reformatting. Something I've run afoul of, having to push changes to PRs because they failed the check. I'd like to take that approach here and not one that changes the code automatically.

I agree. I found that out earlier (changing the formatting is overkill) and changed the PR to only do the check

@2byrds
Copy link
Collaborator Author

2byrds commented Aug 13, 2024

This PR will not be accepted with formatting changes, and as a general open source software contribution practice formatting changes should never be included in PRs that address features or bug fixes. In addition formatting changes that cause files to differ from each other in the same repo should not be submitted.

Please resubmit without the mass reformatting of files so we can review the actual changes.

@pfeairheller I have updated the sections of code that had formatting issues.

@2byrds 2byrds requested a review from pfeairheller August 14, 2024 10:57
@2byrds 2byrds merged commit 832d95a into WebOfTrust:main Aug 20, 2024
5 checks passed
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