Skip to content
This repository has been archived by the owner on Nov 14, 2022. It is now read-only.

Python Skeletonize #9

Open
wants to merge 207 commits into
base: develop
Choose a base branch
from
Open

Python Skeletonize #9

wants to merge 207 commits into from

Conversation

nickviola
Copy link
Contributor

@nickviola nickviola commented Jun 10, 2021

Modernize the repository by skeletonizing it with skeleton-python

🗣 Description

Changes include adding python skeleton, clean up unit tests, enable coveralls, and more.

💭 Motivation and context

This change is required so to get it up to speed with cisagov coding standards

🧪 Testing

Using pytest to test changes and pre-commit

✅ Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

mcdonnnj and others added 30 commits July 13, 2020 09:56
⚠️ CONFLICT! Lineage pull request for: skeleton
All jobs should perform identical work in a consistent manner.
Lineage pull request for: skeleton
Add "requirements.txt" to the actions/cache key for the test job to match what
was done for the lint job. Expand the option to pip in requirements.txt to its
full, more descriptive form. This matches efforts to do this elsewhere in our
codebases.
⚠️ CONFLICT! Lineage pull request for: skeleton
Also use the long form of -e in requirements-test.txt.
Remove the execute bit in file permissions and remove the shebang line. Since
this is configured as a Python package that is installed using pip, it should
only use `python -m example` or the `example` console script to be run.
Lineage pull request for: skeleton
…port

Add Support for Running as Python Module
⚠️ CONFLICT! Lineage pull request for: skeleton
Sort the install_requires and extras_require["test"] arguments by key.
@lgtm-com

This comment has been minimized.

@dav3r
Copy link
Member

dav3r commented Nov 3, 2021

@ameliav - What is the reason for the changes in a455231? We typically prefer list() to [], but if one of our linters says otherwise, I'm interested in seeing that.

@ameliav
Copy link
Contributor

ameliav commented Nov 3, 2021

Hi @dav3r
I have been running "pylint" and below screenshot is what I saw a lot of. I don't mind to change it back if need be.

Screen Shot 2021-11-03 at 3 12 40 PM

@dav3r
Copy link
Member

dav3r commented Nov 3, 2021

Hi @dav3r I have been running "pylint" and below screenshot is what I saw a lot of. I don't mind to change it back if need be.

Screen Shot 2021-11-03 at 3 12 40 PM

Got it- I didn't realize pylint wasn't a fan of list() and dict(). Thanks for sharing!
FYSA: @cisagov/team-ois

@ameliav
Copy link
Contributor

ameliav commented Nov 3, 2021

After looking at this the problem is that the starting value is a floating point per:

height = np.float64(rect.get_height())

If you change this

hours, seconds = divmod(height, 3600)

to

 hours, seconds = divmod(int(height), 3600)

you should be fine to remove the other typecasts.

Hi @mcdonnnj I've made the change and confirmed that it works.

Screen Shot 2021-11-03 at 3 32 00 PM

Resolved in 170652e

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

Update split to only make split happen once with maxsplit because it is only retrieving the first split value in the index anyways
@ameliav
Copy link
Contributor

ameliav commented Nov 4, 2021

@ameliav Per our conversation here's the pylint output to resolve:

************* Module pca_report_library.utility.gets
src/pca_report_library/utility/gets.py:9:43: W0622: Redefining built-in 'type' (redefined-builtin)
src/pca_report_library/utility/gets.py:26:4: C0200: Consider using enumerate instead of iterating with range and len (consider-using-enumerate)
src/pca_report_library/utility/gets.py:9:0: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
************* Module pca_report_library.customer.closing
src/pca_report_library/customer/closing.py:25:34: W0613: Unused argument 'highest_level' (unused-argument)
src/pca_report_library/customer/closing.py:25:49: W0613: Unused argument 'second_level' (unused-argument)
src/pca_report_library/customer/closing.py:227:0: R1711: Useless return at end of function or method (useless-return)
src/pca_report_library/customer/closing.py:303:8: W0622: Redefining built-in 'type' (redefined-builtin)
src/pca_report_library/customer/closing.py:333:45: W0622: Redefining built-in 'type' (redefined-builtin)
src/pca_report_library/customer/closing.py:452:30: W0622: Redefining built-in 'type' (redefined-builtin)
src/pca_report_library/customer/closing.py:477:4: R1705: Unnecessary "elif" after "return" (no-else-return)
************* Module pca_report_library.customer.graphs
src/pca_report_library/customer/graphs.py:103:18: W0612: Unused variable 'value1' (unused-variable)
src/pca_report_library/customer/graphs.py:676:18: W0612: Unused variable 'value1' (unused-variable)
************* Module pca_report_library.customer.generate_report
src/pca_report_library/customer/generate_report.py:130:8: W0621: Redefining name 'replacement' from outer scope (line 127) (redefined-outer-name)
src/pca_report_library/customer/generate_report.py:140:17: R1714: Consider merging these comparisons with "in" to "word2 not in ('User_Click_Summary', 'Complexity')" (consider-using-in)
src/pca_report_library/customer/generate_report.py:203:15: R1701: Consider merging these isinstance calls to isinstance(dictionary[key], (float, int)) (consider-merging-isinstance)
src/pca_report_library/customer/generate_report.py:254:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
src/pca_report_library/customer/generate_report.py:507:4: W0621: Redefining name 'time_gap' from outer scope (line 501) (redefined-outer-name)
src/pca_report_library/customer/generate_report.py:605:20: W0107: Unnecessary pass statement (unnecessary-pass)
src/pca_report_library/customer/generate_report.py:596:18: W0612: Unused variable 'data_value' (unused-variable)
src/pca_report_library/customer/generate_report.py:640:8: W0612: Unused variable 'x' (unused-variable)
src/pca_report_library/customer/generate_report.py:709:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
src/pca_report_library/customer/generate_report.py:713:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
src/pca_report_library/customer/generate_report.py:798:4: R1705: Unnecessary "else" after "return" (no-else-return)
************* Module pca_report_library.compiler.xelatex
src/pca_report_library/compiler/xelatex.py:74:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
src/pca_report_library/compiler/xelatex.py:116:4: R1705: Unnecessary "else" after "return" (no-else-return)

------------------------------------------------------------------
Your code has been rated at 9.82/10 (previous run: 9.82/10, +0.00)

Please let me know if you had any additional questions.

@mcdonnnj All of them should be taken care of. Please review let me know if I missed anything, thanks.

@lgtm-com

This comment has been minimized.

@ameliav
Copy link
Contributor

ameliav commented Nov 9, 2021

Hi @mcdonnnj Please review when you can, thanks.

1 similar comment
@ameliav
Copy link
Contributor

ameliav commented Nov 15, 2021

Hi @mcdonnnj Please review when you can, thanks.

@ameliav
Copy link
Contributor

ameliav commented Nov 18, 2021

Hi @mcdonnnj Please review at your earliest convenience, thanks.

1 similar comment
@ameliav
Copy link
Contributor

ameliav commented Nov 29, 2021

Hi @mcdonnnj Please review at your earliest convenience, thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants