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.

Sign up for GitHub

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

Jump to bottom

feat: report filter by id #144

Merged
merged 2 commits into from
Feb 11, 2024
Merged

feat: report filter by id #144

merged 2 commits into from
Feb 11, 2024

Conversation

tal66
Copy link
Contributor

@tal66 tal66 commented Nov 9, 2023

closes #126

elad-pticha
elad-pticha reviewed Nov 9, 2023
View reviewed changes
src/cmdline.py Outdated Show resolved Hide resolved
@elad-pticha elad-pticha added feature reporter Reporter component labels Nov 9, 2023
elad-pticha
elad-pticha requested changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@elad-pticha elad-pticha left a comment

Choose a reason for hiding this comment

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

LGTM! Please also update the README.md with the the new help menu of the reporter.

@tal66
Copy link
Contributor Author

tal66 commented Nov 9, 2023 •
edited
Loading

done.

  • review: moved the validate function to utils
  • readme: copied from -h

@elad-pticha
Copy link
Contributor

Please rebase from the main branch 🙏

elad-pticha
elad-pticha reviewed Nov 12, 2023
View reviewed changes
src/common/utils.py Outdated Show resolved Hide resolved
@tal66
Copy link
Contributor Author

tal66 commented Nov 12, 2023

did the rebase (and squash) and the last request

@elad-pticha
Copy link
Contributor

Hey @tal66,
Thank you for the changes! A quick update: we are working on a CLA agreement before merging this PR.
We will make sure to keep you updated when we are ready.

@elad-pticha
Copy link
Contributor

Hey @tal66,
We created a CLA for new contributors!
Please take a look at our Contributing Guidelines for more information.

@tal66
Copy link
Contributor Author

tal66 commented Jan 16, 2024

strict :)
sent it just now

@elad-pticha
Copy link
Contributor

Great!
Feel free to rebase and fix issues if there's any 🙏

@elad-pticha elad-pticha self-requested a review January 22, 2024 08:28
elad-pticha
elad-pticha approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@elad-pticha elad-pticha left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

@elad-pticha
Copy link
Contributor

Due to branch protection rules, we require all commits to be signed. Can you please make sure to sign the commit?

@tal66 tal66 closed this Jan 22, 2024
@tal66 tal66 deleted the report-filter-id branch January 22, 2024 11:57
@tal66
Copy link
Contributor Author

tal66 commented Jan 22, 2024

oops PR got closed 'cause i renamed the branch, due to a failing test (branch_prefix="^(feat|fix|refactor|chore|docs|test|library)/[a-z0-9._-]+$"). maybe you can reopen it

@tal66 tal66 restored the report-filter-id branch January 22, 2024 12:02
@elad-pticha elad-pticha reopened this Jan 22, 2024
@elad-pticha
Copy link
Contributor

For now, it is ok that the convention check is failing. However, It is important the commit will be signed.

@tal66
Copy link
Contributor Author

tal66 commented Jan 22, 2024 •
edited
Loading

in my last commit i used git commit --amend --signoff. it appears to be signed in the commit message

@elad-pticha
Copy link
Contributor

Signed commits (using the GPG key) should look like this (contains the verified icon):

image

You can read more here about commit signing.

@tal66
Copy link
Contributor Author

tal66 commented Jan 23, 2024

ok, created a temp key and now the commit appears to be verified (other repos don't require this)

oreenlivnicode
oreenlivnicode requested changes Jan 28, 2024
View reviewed changes
Copy link
Contributor

@oreenlivnicode oreenlivnicode left a comment

Choose a reason for hiding this comment

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

Looks great :)
I have requested one change

src/config/config.py Show resolved Hide resolved
oreenlivnicode
oreenlivnicode approved these changes Feb 4, 2024
View reviewed changes
Copy link
Contributor

@oreenlivnicode oreenlivnicode left a comment

Choose a reason for hiding this comment

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

Great!

@elad-pticha elad-pticha merged commit 66a658e into CycodeLabs:main Feb 11, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oreenlivnicode oreenlivnicode oreenlivnicode approved these changes

@elad-pticha elad-pticha elad-pticha approved these changes

Assignees
No one assigned
Labels
CLA-signed feature reporter Reporter component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filter by id in report
3 participants
@tal66 @elad-pticha @oreenlivnicode