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

many: refactor snapcraft.yaml loading out of load_config #2160

Merged
merged 3 commits into from Jun 19, 2018

Conversation

sergiusens
Copy link
Collaborator

In order to have separated logical bits and clear overlaying
communicated a logical separation is introduced between the
actual snapcraft loading in raw form and its processed/validated
form.

LP: #1775849
Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

In order to have separated logical bits and clear overlaying
communicated a logical separation is introduced between the
actual snapcraft loading in raw form and its processed/validated
form.

LP: #1775849
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
@codecov-io
Copy link

codecov-io commented Jun 15, 2018

Codecov Report

Merging #2160 into master will decrease coverage by 0.05%.
The diff coverage is 95.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2160      +/-   ##
==========================================
- Coverage   91.25%   91.19%   -0.06%     
==========================================
  Files         196      198       +2     
  Lines       12447    12471      +24     
  Branches     1845     1846       +1     
==========================================
+ Hits        11358    11373      +15     
- Misses        734      742       +8     
- Partials      355      356       +1
Impacted Files Coverage Δ
snapcraft/internal/project_loader/errors.py 95.55% <ø> (-0.1%) ⬇️
snapcraft/internal/lifecycle/__init__.py 100% <ø> (ø) ⬆️
snapcraft/project/_project_options.py 89.23% <ø> (ø) ⬆️
snapcraft/integrations/travis.py 92.3% <100%> (ø) ⬆️
snapcraft/cli/ci.py 86.66% <100%> (+2.05%) ⬆️
snapcraft/internal/lifecycle/_clean.py 85.08% <100%> (ø) ⬆️
snapcraft/project/errors.py 100% <100%> (ø)
snapcraft/cli/parts.py 95.65% <100%> (+0.19%) ⬆️
snapcraft/cli/containers.py 93.33% <100%> (ø) ⬆️
snapcraft/project/_get_snapcraft.py 100% <100%> (ø)
... and 20 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 e506e76...94c39f0. Read the comment docs.

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

For such a significant PR, this was surprisingly easy to review. I only have a few suggestions, but nothing large.

self.confinement = data.get('confinement')
self.grade = data.get('grade')
def __init__(self, *, snapcraft_yaml_file_path) -> None:
self.snapcraft_yaml_file_path = get_snapcraft_yaml()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be using snapcraft_yaml_file_path, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

err, yes!

@@ -52,7 +52,13 @@ def _add_build_options(func):
return _add_build_options


def get_project_options(**kwargs):
def get_project(**kwargs):
skip_snapcraft_yaml = kwargs.pop('skip_snapcraft_yaml', False)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cleaner and more intuitive to include this as an actual kwarg so you don't need to pop it:

def get_project(*, skip_snapcraft_yaml: bool=False, **kwargs):
    # ...

return yaml_contents


def get_snapcraft_yaml(base_dir=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest putting this either in snapcraft/project/__init__.py, or in another module imported by __init__.py. Reasoning: it shouldn't be used here, but belongs in the snapcraft/project package somewhere for use elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, which means the tests for it need to be moved as well

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Looks excellent with the changes, thank you!

@sergiusens sergiusens merged commit 44ae843 into canonical:master Jun 19, 2018
@sergiusens sergiusens deleted the project-info branch June 19, 2018 10:57
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.

None yet

3 participants