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

#601 Creation of print_utils.py #716

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

JKnight777
Copy link

Issue 601

Fixes #601

Description

Created a new file called print_utils.py that will help reduce the amount of click.echos in electionguard_cli, while also providing more useful functions that can potentially replace cli_base_step, such as print_header, print_error, print_warning, and print_message.

Added a new python file called print_utils.py to
replace cli_step_base.py and all of the click imports in other files.
Changed the implementation of click in all electionguard_cli files that import click and only use echo/secho, to a new file called print_utils.py.
print_utils.py has functions print_message, print_header, print_error, and print_warning that utilizes click.secho and click.echo to replace
the click.echo and click.secho functions in other files.
@ghost
Copy link

ghost commented Jul 23, 2022

CLA assistant check
All CLA requirements met.

@lgtm-com
Copy link

lgtm-com bot commented Jul 23, 2022

This pull request introduces 10 alerts when merging 45a01f8 into 670b007 - view on LGTM.com

new alerts:

  • 7 for Non-callable called
  • 2 for Unused import
  • 1 for Wrong number of arguments in a call

Finished print_utils.py, and updated cli_step_base.py to incorporate it.
@lgtm-com
Copy link

lgtm-com bot commented Jul 23, 2022

This pull request introduces 9 alerts when merging 9ed7a11 into 670b007 - view on LGTM.com

new alerts:

  • 7 for Non-callable called
  • 1 for Unused import
  • 1 for Wrong number of arguments in a call

@SteveMaier-IRT
Copy link
Collaborator

@JKnight777 Thanks for the submission. The first things that you need to do is to please sign the CLA.

@@ -1,5 +1,5 @@
from typing import Any, Optional
import click
import print_utils

Choose a reason for hiding this comment

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

You can remove the header_color and other colors that are unused in this file

@@ -1,5 +1,5 @@
from typing import List
import click
#import click

Choose a reason for hiding this comment

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

For a polished PR, delete commented out code

@@ -16,7 +15,7 @@
_T = TypeVar("_T")


class InputRetrievalStepBase(CliStepBase):
class InputRetrievalStepBase("""CliStepBase"""):

Choose a reason for hiding this comment

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

I'd like to understand why clickstepbase has triple quotes here...

def print_message(text: str, color="white", underlined=False, bolded=False) -> None:
click.secho(f"{text}", fg=color, underline=underlined, bold=bolded)

def print_warning(text: str, warning_color="bright_red") -> None:

Choose a reason for hiding this comment

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

can you confirm that the warning color and error color are the same?

@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 1 alert when merging bf7d69f into 46dbc34 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

@JKnight777 JKnight777 changed the title Rough Draft for print_utils.py and its implementation #601 Rough Draft for print_utils.py and its implementation Jul 26, 2022
@JKnight777 JKnight777 changed the title #601 Rough Draft for print_utils.py and its implementation #601 Rough Draft for print_utils.py Jul 26, 2022
Copy link

@shivbijlani shivbijlani left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please add proof that it still works with screenshots, or describe your testing.

@JKnight777 JKnight777 marked this pull request as ready for review July 26, 2022 18:40
@JKnight777 JKnight777 changed the title #601 Rough Draft for print_utils.py #601 Creation of print_utils.py Jul 26, 2022
@lgtm-com
Copy link

lgtm-com bot commented Jul 27, 2022

This pull request introduces 1 alert when merging df050d7 into 0dabe5e - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

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.

♻️ Migrate CLI print methods to single module
3 participants