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

Cleanup syntax and linting issues for backend code #577

Merged
merged 3 commits into from Mar 9, 2018

Conversation

mbeacom
Copy link
Contributor

@mbeacom mbeacom commented Mar 8, 2018

Description

The goal of this PR is to cleanup some of the backend code to adhere to flake8/pylint standards.
Additionally, this PR removes many unused files and imports.

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines

Remove unused imports, fix indentations, redundent imports, remove unused files, etc.
@mbeacom mbeacom added backend This needs backend expertise. linting labels Mar 8, 2018
@mbeacom mbeacom self-assigned this Mar 8, 2018
@mbeacom mbeacom requested a review from owocki March 8, 2018 19:04
@owocki
Copy link
Contributor

owocki commented Mar 8, 2018

seems okay to me if it's just linting.. this is gonna cause a lot of merge conflicts.

two questions:

  1. how did you test this, if at all?
  2. is can you update the contrib guidelines with a list of commands i can run to lint my PRs before i submit them?
  3. any updates to travis required?

@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #577 into master will increase coverage by 0.05%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #577      +/-   ##
==========================================
+ Coverage   29.05%   29.11%   +0.05%     
==========================================
  Files          93       88       -5     
  Lines        4890     4880      -10     
  Branches      571      571              
==========================================
  Hits         1421     1421              
+ Misses       3422     3412      -10     
  Partials       47       47
Impacted Files Coverage Δ
app/credits/apps.py 0% <ø> (ø) ⬆️
app/marketing/management/commands/sync_slack.py 0% <ø> (ø) ⬆️
app/app/settings.py 88% <ø> (-0.47%) ⬇️
app/marketing/management/commands/pull_github.py 0% <ø> (ø) ⬆️
...arketing/management/commands/ingest_slack_users.py 0% <ø> (ø) ⬆️
app/legacy/urls.py 100% <ø> (ø) ⬆️
app/gas/admin.py 100% <ø> (ø) ⬆️
app/linkshortener/admin.py 100% <ø> (ø) ⬆️
app/marketing/apps.py 0% <ø> (ø) ⬆️
app/retail/helpers.py 28.57% <ø> (+11.9%) ⬆️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e141dea...bf14ac5. Read the comment docs.

@mbeacom
Copy link
Contributor Author

mbeacom commented Mar 8, 2018

@owocki Agreed, regarding merge conflicts... but we need to get the BE code to a place we can automatically check linting at some point without introducing a stoppage point for every change initiated against the files that were modified here.

This is the first step in one of multiple to get the BE to a point we can lint and not hinder users from contributing without resolving previously existing linting errors.

There are no needed changes to Travis yet, but I can add documentation to the contributing doc regarding linters to run, i.e.: flake8, pydocstyle, pylint, pycodestyle, etc.
I recommend using sublime/atom/vscode plugins to actively monitor these types of errors/warnings while making changes, but most of these can be ran from terminal (but may result in quite a few errors at the moment).

As for testing, I tested this locally and all seemed fine. Most of the changes aren't modifications to logic, but rather some syntax changes and removal of unused imports and skeleton files.

The only thing that should be outstanding following this PR for CI enforced linting should be the functions that violate mccabe checks for complexity, number of local vars, and number of method arguments (from an error standpoint).

Copy link
Contributor

@owocki owocki left a comment

Choose a reason for hiding this comment

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

lgtm

@owocki
Copy link
Contributor

owocki commented Mar 8, 2018

There are no needed changes to Travis yet, but I can add documentation to the contributing doc regarding linters to run, i.e.: flake8, pydocstyle, pylint, pycodestyle, etc.
I recommend using sublime/atom/vscode plugins to actively monitor these types of errors/warnings while making changes, but most of these can be ran from terminal (but may result in quite a few errors at the moment).

+1

@mbeacom mbeacom merged commit 0d87a23 into master Mar 9, 2018
@mbeacom mbeacom deleted the mark-syntax-cleanup branch March 9, 2018 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend This needs backend expertise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants