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

Update black to non pre-release version #73

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

Galileo-Galilei
Copy link
Member

@Galileo-Galilei Galileo-Galilei commented Mar 8, 2022

Motivation and Context

Upgrade black to 22.1.0 consistently with kedro-org/kedro#1327.

How has this been tested?

  • I have run black on the entire repo. Notice that some files return an error because of the jinja tags which are not valid python.
  • I also manually created a project fom each starter and run black and one single file is sometimes modified: the setup.py because the length of line with the entry point depends on the length of the project, which we don't know in advance. There are not other modifcation.
  • As for the original repo, I did NOT change black version in test_requirements.txt. Should I?

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Assigned myself to the PR -> I don't have the right to do it
  • Added tests to cover my changes -> should I create a test to check if the template is "black valid"? What should I do for the lines which are splitted depending on the length of the project name?

Signed-off-by: Yolan Honoré-Rougé <yolan.honore.rouge@gmail.com>
@Galileo-Galilei Galileo-Galilei changed the title update black and run it on starters Update black to non pre-release version Mar 8, 2022
@antonymilne
Copy link
Contributor

Thanks very much for doing this @Galileo-Galilei! And especially for your diligent testing.

In response to your points:

The problem here is that for some reason your PR hasn't triggered any of our CI checks. You should be seeing something like this:
image

I think @MerelTheisenQB might know what the solution is here to get the checks to execute on this PR.

Also N.B. @lorenabalan I advised that this PR should go into your 0.18.0 branch since it relies on the recently bumped click version requirement, which is only in develop. Hope that's right.

@antonymilne
Copy link
Contributor

Forget what I said about the CircleCI jobs actually - we just hadn't enabled them to run on forked PRs. It's been enabled and I've done a PR to trigger them now 😄

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Just don't forget to update test_requirements.txt 🙂

Signed-off-by: Yolan Honoré-Rougé <yolan.honore.rouge@gmail.com>
@antonymilne antonymilne merged commit 64efe3a into kedro-org:0.18.0 Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants