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.

Sign up for GitHub

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

Jump to bottom

Improve Code Style #119

Merged
merged 14 commits into from
Mar 20, 2022
Merged

Improve Code Style #119

merged 14 commits into from
Mar 20, 2022

Conversation

rohanbabbar04
Copy link
Contributor

Description

  • In signac_dashboard/__main__.py, removed append
  • Replaced Mutable default arguments in signac_dashboard/modules/image_viewer.py and signac_dashboard/modules/video_viewer.py
  • Renaming in signac_dashboard/modules/notes.py and signac_dashboard/dashboard.py

Motivation and Context

These are some of the warnings which can be improved upon

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog.

@rohanbabbar04 rohanbabbar04 requested review from bdice and a team as code owners March 8, 2022 17:43
@bdice
Copy link
Member

bdice commented Mar 8, 2022

@rohanbabbar04 Are you running a tool that made these suggestions? I'm not sure what warnings you were seeing.

bdice
bdice reviewed Mar 8, 2022
View reviewed changes
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I have a couple minor suggestions for improvement.

signac_dashboard/modules/image_viewer.py Outdated
@@ -35,10 +35,12 @@ def __init__(
name="Image Viewer",
context="JobContext",
template="cards/image_viewer.html",
img_globs=["*.png", "*.jpg", "*.gif"],
img_globs=None,
Copy link
Member

Choose a reason for hiding this comment

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

If the goal is to make this argument immutable, we can use a tuple rather than None. Tuples are immutable. I would like to keep the default argument something that has a clear value, since it appears in the documentation.

Suggested change
img_globs=None,
img_globs=("*.png", "*.jpg", "*.gif"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted

signac_dashboard/modules/video_viewer.py Outdated
@@ -50,12 +50,14 @@ def __init__(
name="Video Viewer",
context="JobContext",
template="cards/video_viewer.html",
video_globs=["*.mp4", "*.m4v"],
video_globs=None,
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, use a tuple for an immutable argument.

Suggested change
video_globs=None,
video_globs=("*.mp4", "*.m4v"),

@bdice
Copy link
Member

bdice commented Mar 19, 2022

@rohanbabbar04 Are you running a tool that made these suggestions? I'm not sure what warnings you were seeing.

Hi @rohanbabbar04, I need a little more information about where you're seeing these warnings before reviewing.

@rohanbabbar04
Copy link
Contributor Author

@rohanbabbar04 Are you running a tool that made these suggestions? I'm not sure what warnings you were seeing.

Hi @rohanbabbar04, I need a little more information about where you're seeing these warnings before reviewing.

I will not say like warnings but you can say python coding style(practice)

@bdice
Copy link
Member

bdice commented Mar 19, 2022

@rohanbabbar04 Did you just read the entire code base and look for things to change or did you use some kind of automated tool? We use the pre-commit framework to automate most of the package’s requirements related to code quality. If these suggestions come from a tool, can that tool be added to the pre-commit configuration?

@bdice
Copy link
Member

bdice commented Mar 19, 2022 •
edited

Since this PR doesn’t actually fix any warnings, a more descriptive title would be helpful. Perhaps “Improve code style.”

@rohanbabbar04 rohanbabbar04 changed the title Removed warnings Improve Code Style Mar 20, 2022
@rohanbabbar04
Copy link
Contributor Author

Since this PR doesn’t actually fix any warnings, a more descriptive title would be helpful. Perhaps “Improve code style.”

Done

@bdice bdice enabled auto-merge (squash) March 20, 2022 21:44
@bdice bdice merged commit 310a0ca into glotzerlab:master Mar 20, 2022
javierbg pushed a commit to javierbg/signac-dashboard that referenced this pull request Nov 28, 2022
@rohanbabbar04 @pre-commit-ci @bdice
Improve Code Style (glotzerlab#119)
3e23fab 
* Updated Requirements.txt

* Initialization Warning Solved

* Renaming solved

* Initialization Warning Removed

* Renamed assetfile to asset_file

* Three expressions can be compared together Removing and

* Reverted reqs

* Reverted reqs

* Added a tuple instead of None

* Revert "Added a tuple instead of None"

This reverts commit 5e56b44.

* Changed None to a tuple

* Indentation solved

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Trigger CI.

Co-authored-by: rohanbabbar <rohanbabbar0408@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rohanbabbar04 @bdice