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

Remove mockings #3378

Merged
merged 7 commits into from
Nov 11, 2022
Merged

Remove mockings #3378

merged 7 commits into from
Nov 11, 2022

Conversation

hjoaquim
Copy link
Contributor

This PR aims to remove the function openbb_terminal/core/log/generation/formatter_with_exceptions.mock_break_lines.

In the way logging is working at the moment, there is no real benefit on having this function, since the logs that depend on the user input are already addressed by:

openbb_terminal/parent_classes.log_cmd_and_queue and openbb_terminal/core/library/operation.__log_method_info --> using json.dumps ensure the string is in a valid json format.

Also, by changing the \s in an already valid json string we take the risk of making the json string invalid thus, eventually affecting the ETL pipeline or the queries - that expect a well-formed json string.

@hjoaquim hjoaquim added the analytics Logging and analytics label Nov 10, 2022
@hjoaquim hjoaquim self-assigned this Nov 10, 2022
Copy link
Contributor

@Chavithra Chavithra left a comment

Choose a reason for hiding this comment

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

Looks ok except for the line that should be added.

@hjoaquim hjoaquim added the do not merge Label to prevent pull request merge label Nov 10, 2022
@hjoaquim hjoaquim removed the do not merge Label to prevent pull request merge label Nov 11, 2022
@hjoaquim hjoaquim merged commit 9d26d77 into OpenBB-finance:main Nov 11, 2022
@hjoaquim hjoaquim deleted the remove_mockings branch November 11, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analytics Logging and analytics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants