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

Autofmt #71

Closed
wants to merge 3 commits into from
Closed

Autofmt #71

wants to merge 3 commits into from

Conversation

bolliger32
Copy link
Member

@bolliger32 bolliger32 commented Jul 11, 2020

  • tests added / passed (can't figure out what's going on with the sphinx error...builds ok on my notebook image)
  • docs reflect changes (N/A)
  • passes flake8 rhg_compute_tools tests docs (some things in tests still raise flake8 warnings, but they are ones that are needed for pytest)
  • entry in HISTORY.rst (N/A

Started off by just calling black and flake8 on everything. Flake8 caused me to spot a few typos that hadn't caught up to us yet but would soon.

  • isintance instead of isinstance in kubernetes.py
  • had not imported warnings in kubernetes.py
  • weren't removing all the rst files we wanted to when re-generating sphinx apidocs

Copy link
Member

@brews brews left a comment

Choose a reason for hiding this comment

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

Yo, @bolliger32! I'm happy to give this a review this week. Could you give me a quick summary of what's going on and the goals here, I want to be sure I'm on the same page.

So we're hitting all the files with black and flake8 and we're getting the cleaned up files here. What rules/configs were used for the black and flake8?

What is the thinking about making black and flake8 checks part of an early test stage in CI/CD? i.e.: CI/CD rejects code changes unless it follows style?

Edit:
Also, I noticed our CI/CD check are already failing this. Is this something we should open a new issue for or is there a larger change this is a part of?

@bolliger32
Copy link
Member Author

Hey @brews !

Yeah the summary is just that I started off just trying to clean things up just calling black on everything. We've added black --check . as part of our testing on the coastal team repo and I think it's helped keep things fairly uniform. So i'd be in favor of that if you guys want. Regardless, for the time being, I just thought I would tidy things up a bit. In addition to black, I also ran flake8 just to see what it would come up with (just default settings, except for --max-line-length=88 to match black). I didn't fix everything I dont' think (e.g. I didn't fix all whitespaces) but I fixed the typos it noticed (like when I wrote isintance instead of isinstance).

Previously master was failing so I thought that was the failures that this PR was running into, but I just rebased onto @delgadom 's changes (which are now passing on master) and it's still failing. Not sure what's going on as it's a sphinx-build issue that passes fine on my notebook...

Copy link
Member

@delgadom delgadom left a comment

Choose a reason for hiding this comment

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

In general reviewing "black everything" PRs is close to impossible so I'd strongly prefer separating this from everything else. Ok if we close this, run black as a separate PR, then try to tackle these remaining items one at a time?

.travis.yml Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
@delgadom delgadom mentioned this pull request Jul 14, 2020
@bolliger32
Copy link
Member Author

In general reviewing "black everything" PRs is close to impossible so I'd strongly prefer separating this from everything else. Ok if we close this, run black as a separate PR, then try to tackle these remaining items one at a time?

Soundsd good to me! I'll keep this branch and then when we're ready to try again after theres a separate black-only PR we can merge again.

@bolliger32
Copy link
Member Author

closing - see #75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants