parser: support remote dependencies #930

Merged
merged 4 commits into from Dec 2, 2016

Conversation

Projects
None yet
3 participants
Contributor

3v1n0 commented Nov 28, 2016

Ensure that we re-parse the parts that have missing dependencies after we've added all the valid ones to the master_parts_list

I would have fixed this in a cleaner way (allowing to include parts of a snapcraft.yaml that are good, and ignoring the bad ones only) but it would have caused a major refactor of the parser which probably is better to avoid.

Fixes LP: #1645350

@josepht, please have a look to this.

Contributor

josepht commented Nov 28, 2016

The approach looks good to me. Thanks for working on this. Please fix the complexity issue and check that the coverage for the files you've modified hasn't decreased and that all new code is covered. Coverage can be a bit wonky at times. :)

Contributor

3v1n0 commented Nov 28, 2016

@josepht

Please fix the complexity issue

I'm not sure how to deal with that, should I just split the function? :o

Check that the coverage for the files you've modified hasn't decreased

This is due to the exception that I protect from in the for-cycle over pending_validation_entries.
The fact is that I don't think is possible that an exception might be triggered there, but wasn't wrong to protect, isn't it?
Otherwise I'd just drop the try-except.

Contributor

josepht commented Nov 28, 2016

I think you can split out the post processing for the pending entries into its own method.

You can probably trigger that exception by referencing a pending part that has a missing 'parts' in the yaml. I think we want to keep the try - except since _process_wiki_entry() raises a couple of exceptions.

Contributor

josepht commented Nov 29, 2016

Nice job killing two birds with one stone. :-) +1

3v1n0 added some commits Nov 24, 2016

parser: check if a part is already known before warn
This allows to support remote parts that have a dependency
on other remotes.

This still depends on wiki ordering, though.
Parser: try to re-evaluate failed parts
If some parts are not valid because they refer
to some undefined "after" parts we try to reparse
them at the end.

@sergiusens sergiusens changed the title from Parser support remote dependencies to parser: support remote dependencies Nov 30, 2016

Thanks for this, I have a feeling this would be easier to follow if we make it less procedural and more OO. What do you think?

If @josepht approved I am fine with it though

Contributor

3v1n0 commented Dec 1, 2016

@sergiusens completely agree... And in fact so it was "almost" stating my initial comment on PR first post, but things were already so much procedural, so I didn't want to rewrite everything.
But I guess is something that has to be done, sooner or later (but it wasn't something for me this time :-P).

@sergiusens sergiusens merged commit 6c01219 into snapcore:master Dec 2, 2016

5 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 98.21%
Details
xenial-amd64 autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

parser: support remote dependencies (#930)
Ensure that we re-parse the parts that have missing dependencies after we've added all the valid ones to the master_parts_list

I would have fixed this in a cleaner way (allowing to include parts of a snapcraft.yaml that are good, and ignoring the bad ones only) but it would have caused a major refactor of the parser which probably is better to avoid.

LP: #1645350
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment