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

Unit Test Cases For fill_stats.py added #1012

Merged
merged 17 commits into from
Jun 11, 2024
Merged

Unit Test Cases For fill_stats.py added #1012

merged 17 commits into from
Jun 11, 2024

Conversation

surbhi498
Copy link
Contributor

@surbhi498 surbhi498 commented May 29, 2024 •
edited
Loading

Unit Test Cases For file fill_stats.py added in test file test_fill_stats.py.
#1001 (comment)

DanilYachmenev
DanilYachmenev reviewed May 30, 2024
View reviewed changes
Copy link
Collaborator

@DanilYachmenev DanilYachmenev left a comment

Choose a reason for hiding this comment

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

pls mention a link to issue in PR description for easier navigation for reviewers

read carefully
https://github.com/kaizen-ai/kaizenflow/blob/master/docs/coding/all.submit_code_for_review.how_to_guide.md#save-reviewer-time

you final goal is to save reviewers time

core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
@DanilYachmenev DanilYachmenev added the PR_for_authors The PR needs changes label May 30, 2024
@surbhi498 surbhi498 mentioned this pull request Jun 1, 2024
Open
@surbhi498 surbhi498 requested a review from DanilYachmenev June 2, 2024 01:27
@surbhi498 surbhi498 added PR_for_reviewers The PR needs to be reviewed by Team Leaders and removed PR_for_authors The PR needs changes labels Jun 2, 2024
gpsaggese
gpsaggese reviewed Jun 3, 2024
View reviewed changes
Copy link
Contributor

@gpsaggese gpsaggese left a comment

Choose a reason for hiding this comment

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

Small nits to fix, but ok to merge and keep moving

core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
DanilYachmenev
DanilYachmenev reviewed Jun 3, 2024
View reviewed changes
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
@DanilYachmenev DanilYachmenev added PR_for_authors The PR needs changes and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Jun 3, 2024
@surbhi498 surbhi498 requested a review from DanilYachmenev June 5, 2024 02:40
@surbhi498 surbhi498 added PR_for_reviewers The PR needs to be reviewed by Team Leaders and removed PR_for_authors The PR needs changes labels Jun 5, 2024
@surbhi498 surbhi498 requested a review from gpsaggese June 5, 2024 02:49
DanilYachmenev
DanilYachmenev reviewed Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@DanilYachmenev DanilYachmenev left a comment

Choose a reason for hiding this comment

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

run linter, fix docstring and ready to merge

core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
core/finance/test/test_fill_stats.py Outdated Show resolved Hide resolved
@DanilYachmenev DanilYachmenev added PR_for_authors The PR needs changes and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Jun 5, 2024
@surbhi498 surbhi498 requested a review from DanilYachmenev June 5, 2024 20:43
@surbhi498 surbhi498 added PR_for_reviewers The PR needs to be reviewed by Team Leaders and removed PR_for_authors The PR needs changes labels Jun 5, 2024
DanilYachmenev
DanilYachmenev reviewed Jun 7, 2024
View reviewed changes
Copy link
Collaborator

@DanilYachmenev DanilYachmenev left a comment

Choose a reason for hiding this comment

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

lgtm

@DanilYachmenev DanilYachmenev added PR_for_integrators The PR needs to be reviewed and possibly merged and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Jun 7, 2024
@samarth9008 samarth9008 merged commit 8ca212e into master Jun 11, 2024
1 check passed
@samarth9008 samarth9008 deleted the SorrTask1001_Test_compute_fill_stats branch June 11, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanilYachmenev DanilYachmenev DanilYachmenev left review comments

@samarth9008 samarth9008 Awaiting requested review from samarth9008

@gpsaggese gpsaggese Awaiting requested review from gpsaggese

Assignees

@surbhi498 surbhi498

Labels
PR_for_integrators The PR needs to be reviewed and possibly merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@surbhi498 @DanilYachmenev @gpsaggese @samarth9008