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

Improved fraud function #1855

Merged
merged 8 commits into from
May 26, 2022
Merged

Improved fraud function #1855

merged 8 commits into from
May 26, 2022

Conversation

colin99d
Copy link
Contributor

@colin99d colin99d commented May 22, 2022

Description

  • Summary of the change / bug fix.
  • Only shows sub calculations for MSCORE if they are requested (these are confusing)
  • Adds color to show good, medium, and bad (makes numbers meaningful to user
  • Link # issue, if applicable.
  • Fixes Fraud Explanations[IMPROVE] #1727
  • Screenshot of the feature or the bug before/after fix, if applicable.

Screen Shot 2022-05-22 at 2 12 24 PM

How has this been tested?

Ran pytest

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@colin99d colin99d requested a review from JerBouma May 22, 2022 18:12
@DidierRLopes DidierRLopes added bug Fix bug enhancement Enhancement labels May 22, 2022
@@ -251,6 +270,7 @@ def display_fraud(ticker: str, export: str = "", help_text: bool = False):
if help_text:
console.print(help_message)
export_data(export, os.path.dirname(os.path.abspath(__file__)), "dupont", df)
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just doing it to match the other return so pylint would not throw an error.

default=False,
help="Shows the details for calculating the mscore",
)
parser.add_argument(
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's have it true by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? These calculations are kind of "in the weeds" and will not be valuable to most people.

ticker: str,
export: str = "",
help_text: bool = False,
color: bool = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's have color True and not being optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@DidierRLopes DidierRLopes merged commit 75d0832 into main May 26, 2022
@DidierRLopes DidierRLopes deleted the fraud_desc branch May 30, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fraud Explanations[IMPROVE]
2 participants