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

.ci/generate_bear_requirements: Changes #2446

Merged
merged 1 commit into from
Jul 13, 2018
Merged

Conversation

nemani
Copy link
Member

@nemani nemani commented Apr 19, 2018

TODO: break into commits.
This commits updates the script to use ruamel.yaml
which is better fork of PyYaml.
Add Update and Check mode for diffing and updating
Bear-Requirments yaml.

Closes #2444

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

  • I read the commit guidelines and I've followed
    them.
  • I ran coala over my code locally. (All commits have to pass
    individually.
    It is not sufficient to have "fixup commits" on your PR,
    our bot will still report the issues for the previous commit.) You will
    likely receive a lot of bot comments and build failures if coala does not
    pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

@nemani
Copy link
Member Author

nemani commented Apr 19, 2018

@jayvdb this should be it. :))

@nemani
Copy link
Member Author

nemani commented Apr 22, 2018

@jayvdb Can you drop this a small review? I have exams this week, and wont get much time to refine the PR. But all necessary functionality is working IMO.

Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

there are no pypi packages which provide deepupdate/deepdiff ?

@@ -1,3 +1,84 @@
# This is a comment!
overrides: coala-build.yaml
pip_requirements:
Copy link
Member

Choose a reason for hiding this comment

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

can you modify the generator so this doesnt move in the yaml.
It is easier to verify the code is correct than verify the moved yaml is identical

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? The pip_requirements part? or the overrides part?

@@ -1,3 +1,84 @@
# This is a comment!
Copy link
Member

Choose a reason for hiding this comment

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

obviously not wanted after WIP

parser.add_argument('--check', '-c', action='store_true',
help='performs a dry run, and reports differences.')
parser.add_argument('--update', '-u',action='store_true',
help='updates "bear-requirements.yaml" instaead of overwriting')
Copy link
Member

Choose a reason for hiding this comment

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

instaead

and 80cpl

@jayvdb
Copy link
Member

jayvdb commented Apr 22, 2018

Nice work so far, showing the principle idea will work.
Leave it now, until after the exams.

.travis.yml Outdated
@@ -115,7 +115,8 @@ script:
- pip install $(ls ./dist/*.whl)"[alldeps]"
- bash .ci/tests.sh
# Ensure bear requirements are in sync with the bear PipRequirement
- .ci/generate_bear_requirements.py
- .ci/generate_bear_requirements.py --check
Copy link
Member

Choose a reason for hiding this comment

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

This should be --check --update in the same invocation to speed up the build.

requirements['gem_requirements'] = get_gem_requirements(gem_reqs)
requirements['npm_requirements'] = get_npm_requirements(npm_reqs)
requirements['pip_requirements'] = get_pip_requirements(pip_reqs)
Copy link
Member

Choose a reason for hiding this comment

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

keep the order : pip ; npm ; gem

then these lines wont appear as moved.

requirements['pip_requirements'] = get_pip_requirements(pip_reqs)

if args.update or args.check:
inputFile = open(os.path.join(PROJECT_DIR, BEAR_REQUIREMENTS_YAML),'r')
Copy link
Member

Choose a reason for hiding this comment

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

pep8 variable names please

target[key] = copy.copy(value)


def deepdiff(target, src):
Copy link
Member

Choose a reason for hiding this comment

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

deep_diff

errors = []
for key, value in src.items():
if not key in target:
errors.append((key, "Missing"))
Copy link
Member

Choose a reason for hiding this comment

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

bad indentation; single quote.

@@ -167,6 +176,46 @@ def get_pip_requirements(requirements):

return pip_requirements

def deepupdate(target, src):
Copy link
Member

Choose a reason for hiding this comment

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

deep_update

@@ -30,6 +31,9 @@
from dependency_management.requirements.NpmRequirement import NpmRequirement
from dependency_management.requirements.PipRequirement import PipRequirement

yaml = YAML(typ="rt")
Copy link
Member

Choose a reason for hiding this comment

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

single quotes is our preferred style now

@@ -18,9 +18,10 @@
import json
import os
import sys
from collections import OrderedDict
import copy
Copy link
Member

Choose a reason for hiding this comment

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

alpha order

for key, value in src.items():
if isinstance(value, list):
if not key in target:
target[key] = copy.deepcopy(value)
Copy link
Member

Choose a reason for hiding this comment

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

It is simpler if you always do this, irrespective of the type

if not key in target:
    target[key] = copy.deepcopy(value)

Then in the else you can do the different update mechanisms.
That makes it more clear that this is like a deepcopy for any new keys, but behaves differently for other keys, and raise an exception if there is an update required for an unknown type as we will need to add support for it whenever that happens.

@@ -9,3 +9,4 @@ pytest-timeout~=1.2
pytest-xdist~=1.15
requests_mock~=0.7.0
wheel~=0.29
ruamel.yaml==0.15.37
Copy link
Member

Choose a reason for hiding this comment

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

ruamel.yaml~=0.15.38

.travis.yml Outdated
@@ -150,7 +150,10 @@ script:
- pip install $(ls ./dist/*.whl)"[alldeps]"
- bash .ci/tests.sh
# Ensure bear requirements are in sync with the bear PipRequirement
- .ci/generate_bear_requirements.py
- .ci/generate_bear_requirements.py --check --update
- git clone https://github.com/moremoban/setupmobans ../setupmobans
Copy link
Member

Choose a reason for hiding this comment

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

these three lines were moved to a separate job

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@jayvdb jayvdb mentioned this pull request Jun 22, 2018
@jayvdb jayvdb changed the title [WIP] .ci/generate_bear_requirements: Changes .ci/generate_bear_requirements: Changes Jun 22, 2018
@jayvdb
Copy link
Member

jayvdb commented Jun 22, 2018

Need to rebase on top of #2542 to get into the queue (current 3 in the queue waiting to be merged)

@jayvdb
Copy link
Member

jayvdb commented Jun 22, 2018

@gitmate-bot rebase

@gitmate-bot
Copy link
Collaborator

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@gitmate-bot
Copy link
Collaborator

Automated rebase with GitMate.io was successful! 🎉

@jayvdb
Copy link
Member

jayvdb commented Jul 5, 2018

And rebase your PR before uploading!

requirements = {}
requirements = CommentedMap()
requirements.yaml_set_start_comment(
"This is an automatically generated file.\n "
Copy link
Member

Choose a reason for hiding this comment

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

indent four spaces only and use single quotes.

and rebase again ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

try:
input_file = open(input_file_path, 'r')
except FileNotFoundError:
print("bear-requirements.yaml not found. "
Copy link
Member

Choose a reason for hiding this comment

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

single quotes.

note #2584 . Stop using me as your personal linter and instead use coala-ci to do that for you :P

Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

There are quite a few places where comments are stripped in "update" mode, especially anywhere around the "foo: true" gems. but we have #2561 for those.

This is good enough to begin with, as we will be able to add comments per requirement and them not disappear.

@jayvdb
Copy link
Member

jayvdb commented Jul 13, 2018

ack d253c61

This commits updates the script to use ruamel.yaml
which is better fork of PyYaml.
Also adds Update and Check mode for diffing and
updating Bear-Requirements yaml.

Closes coala#2444
@jayvdb
Copy link
Member

jayvdb commented Jul 13, 2018

@gitmate-bot ff

@gitmate-bot
Copy link
Collaborator

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

@gitmate-bot
Copy link
Collaborator

Automated fastforward with GitMate.io was successful! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Allow extra information in bear-requirements.yaml
4 participants