FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

#601 Creation of print_utils.py by JKnight777 · Pull Request #716 · Election-Tech-Initiative/electionguard-python · GitHub

/ electionguard-python Public

#601 Creation of print_utils.py - #716

Open
JKnight777 wants to merge 7 commits into
Election-Tech-Initiative:mainfrom
JKnight777:main
ConversationCommits (7)ChecksFiles changed
Open

#601 Creation of print_utils.py#716
JKnight777 wants to merge 7 commits into
Election-Tech-Initiative:mainfrom
JKnight777:main

Conversation

JKnight777 commented Jul 23, 2022

Copy link
Copy Markdown

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.

Co-authored-by: Antonio Lopez Jr <Alopezjr2002@users.noreply.github.com>
8379684 
Co-authored-by: Pedro Rangel <CodingForFun-coder@users.noreply.github.com>
Rough Draft:
be80856 
Added a new python file called print_utils.py to
replace cli_step_base.py and all of the click imports in other files.
Rough Draft for print_utils.py:
8d2d198 
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 commented Jul 23, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown


All CLA requirements met.

Co-authored-by: Pedro Rangel <CodingForFun-coder@users.noreply.github…
45a01f8 
….com>

Co-authored-by: Antonio Lopez Jr <Alopezjr2002@users.noreply.github.com>

lgtm-com Bot commented Jul 23, 2022

Copy link
Copy Markdown

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

Rough Draft for print_utils.py
9ed7a11 
Finished print_utils.py, and updated cli_step_base.py to incorporate it.

lgtm-com Bot commented Jul 23, 2022

Copy link
Copy Markdown

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 commented Jul 25, 2022

Copy link
Copy Markdown
Contributor

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

shivbijlani reviewed Jul 25, 2022
View reviewed changes
Comment thread src/electionguard_cli/cli_steps/cli_step_base.py
@@ -1,5 +1,5 @@
from typing import Any, Optional
import click
import print_utils

shivbijlani Jul 25, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

shivbijlani reviewed Jul 25, 2022
View reviewed changes
Comment thread src/electionguard_cli/e2e/submit_votes_step.py Outdated
@@ -1,5 +1,5 @@
from typing import List
import click
#import click

shivbijlani Jul 25, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For a polished PR, delete commented out code

shivbijlani reviewed Jul 25, 2022
View reviewed changes
Comment thread src/electionguard_cli/cli_steps/input_retrieval_step_base.py Outdated


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

shivbijlani Jul 25, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

shivbijlani reviewed Jul 26, 2022
View reviewed changes
Comment thread src/electionguard_cli/print_utils.py
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:

shivbijlani Jul 26, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lgtm-com Bot commented Jul 26, 2022

Copy link
Copy Markdown

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 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 changed the title #601 Rough Draft for print_utils.py and its implementation #601 Rough Draft for print_utils.py Jul 26, 2022
shivbijlani approved these changes Jul 26, 2022
View reviewed changes

shivbijlani left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

lgtm-com Bot commented Jul 27, 2022

Copy link
Copy Markdown

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
shivbijlani shivbijlani approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

♻️ Migrate CLI print methods to single module

3 participants

Back | FazBrowse Home | New Git URL