/snapcraft

parser - Remove namespacing and subparts. #705

Merged
merged 15 commits into from Aug 23, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@josepht @sergiusens @elopio @kyrofa
@josepht
Contributor

josepht commented Aug 1, 2016 •
Edited 1 time

  • remove 'project-part' from wiki entries
  • require 'parts' in wiki entries
  • 'parts' should have what was the original 'project-part' plus any
    other parts

LP:#1606933

Signed-off-by: Joe Talbott joe.talbott@ubuntu.com
@josepht
Contributor

josepht commented Aug 2, 2016

Should this change be broadcast to the mailing list prior to landing so that our growing community of remote parts authors and users are aware of the changes?
@sergiusens
Collaborator

sergiusens commented Aug 3, 2016

retest this please

@josepht josepht referenced this pull request in Ubuntu/snapcraft-desktop-helpers Aug 10, 2016

Merged

Update for pending sub-parts namespacing removal. #5

snapcraft/internal/parser.py
- return
+ parts = data.get('parts', [])
+ for part_name in parts:
+ if part_name is not None and part_name in master_parts_list:
@sergiusens

sergiusens Aug 16, 2016

Collaborator

this would be easier to read

if part_name and part_name in master_parts_list:
@josepht

josepht Aug 16, 2016

Contributor

Fixed.

snapcraft/tests/test_parser.py
+ self.assertEqual(2, _get_part_list_count())
+
+ part1 = _get_part('curl/a')
+ self.assertTrue(part1 is not None)
@sergiusens

sergiusens Aug 16, 2016

Collaborator

is not None shouln't be needed here.

@josepht

josepht Aug 16, 2016

Contributor

Fixed.

snapcraft/tests/test_parser.py
+ self.assertTrue(part1 is not None)
+
+ part2 = _get_part('curl-a')
+ self.assertTrue(part2 is not None)
@sergiusens

sergiusens Aug 16, 2016

Collaborator

ditto

@josepht

josepht Aug 16, 2016

Contributor

Fixed.

@sergiusens
Collaborator

sergiusens commented Aug 16, 2016

I assume this part of the comment is no longer valid?
Hack to handle 'origin_dir' until the origin encoded path lands in
PR #618
@josepht
Contributor

josepht commented Aug 16, 2016

@sergiusens thanks for catching that. I've removed it from the PR comment.

josepht added some commits Jul 28, 2016

@josepht
parser - Remove namespacing and subparts. 
* remove 'project-part' from wiki entries
* require 'parts' in wiki entries
* 'parts' should have what was the original 'project-part' plus any
  other parts

* Hack to handle 'origin_dir' until the origin encoded path lands in
  PR#618

LP:#1606933

Signed-off-by: Joe Talbott <joe.talbott@ubuntu.com>
5987bf6
@josepht
Convert the remaining 'project-parts' and document.
3f3f874
@josepht
Add unit test for slashes and improve coverage
13dbf49
@josepht
Don't fix coverage issues here
dac06f2
@josepht
Simplify conditionals.
6cc8785
@josepht
Fix tests to conform with proper error codes.
a3372d1
@josepht
Improve coverage

Some checks were not successful

1 failing and 3 successful checks
coverage/coveralls — Coverage decreased (-0.01%) to 97.464%
Details
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
77b92ca
@josepht
Add a deprecation warning for '/' in part names.

Some checks were not successful

1 failing and 3 successful checks
coverage/coveralls — Coverage decreased (-0.0008%) to 97.473%
Details
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
b9c925c
@josepht
Get rid of unused caching bits from fake parts wiki

All checks have passed

4 successful checks
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.04%) to 97.519%
Details
49d28e9
snapcraft/internal/parser.py
+ for part in entry_parts:
+ if '/' in part:
+ logger.warning(
+ 'A "/" in a part name is deprecated and will be removed')
@sergiusens

sergiusens Aug 22, 2016

Collaborator

Seems like the wording here could use some work.

@sergiusens

sergiusens Aug 22, 2016

Collaborator

Maybe
DEPRECATED: Found a "/" in the name of the {!r} part.format(part_name)

snapcraft/internal/parser.py
after_parts = set()
parts_list = {}
- for part in subparts:
+ for part in entry_parts:
@sergiusens

sergiusens Aug 22, 2016

Collaborator

would it be too much to ask you to call part part_name?

snapcraft/tests/fake_servers.py
@@ -138,12 +161,8 @@ class FakePartsWikiOriginRequestHandler(BaseHTTPRequestHandler):
def do_GET(self):
logger.debug('Handling getting part origin')
- if self.headers.get('If-None-Match') == '1111':
@sergiusens

sergiusens Aug 22, 2016

Collaborator

Are we testing the update not needed scenario still?

@josepht

josepht Aug 22, 2016

Contributor

This is only for the parts wiki origin tests not the remote parts. I copied it over when I created these fake servers and forgot to remove that line.

snapcraft/internal/parts.py
@@ -183,6 +183,9 @@ def __init__(self, parts_data, project_options, validator, build_tools,
def _process_parts(self):
for part_name in self._parts_data:
+ if '/' in part_name:
+ logger.warning(
+ 'A "/" in a part name is deprecated and will be removed')
@sergiusens

sergiusens Aug 22, 2016

Collaborator

same thing about the warning text

@sergiusens
Collaborator

sergiusens commented Aug 22, 2016

@elopio want to take a look at this one too?

josepht added some commits Aug 22, 2016

@josepht
Merge branch 'master' into bugs/1606933
0056b07
@josepht
Clean up wording and variable names 
- s/part/part_name/ for clarity
- Use sergiusens' suggestion for deprecation warning text.

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
autopkgtest integration
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
@snappy-m-o
autopkgtest snaps — Success
Details
coverage/coveralls — Coverage increased (+0.04%) to 97.498%
Details
74f5482
@josepht
Fix integration tests for proper return value.

All checks have passed

4 successful checks
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.04%) to 97.498%
Details
7bbd02d
docs/snapcraft-parts.md
* description - A brief description of the part.
+ * parts - A YAML list of other parts from the snapcraft.yaml that are needed.
@elopio

elopio Aug 22, 2016

Member

I think now we need to remove the word "other" from here, right?

@josepht

josepht Aug 22, 2016

Contributor

Right indeed, fixed.

snapcraft/internal/parser.py
@@ -63,7 +63,6 @@ class WikiError(Exception):
logger = logging.getLogger(__name__)
-PART_NAMESPACE_SEP = '/'
@elopio

elopio Aug 22, 2016

Member

bye bye solidus :D

@josepht
Fix documentation for 'parts'

All checks have passed

4 successful checks
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.04%) to 97.498%
Details
f76ee99
snapcraft/internal/parts.py
@@ -189,6 +189,10 @@ def _remote_parts(self):
def _process_parts(self):
for part_name in self._parts_data:
+ m = 'DEPRECATED: Found a "/" in the name of the {!r} part'.format(
+ part_name)
@elopio

elopio Aug 22, 2016

Member

just a nit, but this variable should be inside the if block.

@elopio
Member

elopio commented Aug 22, 2016

lgtm. Just two nits, and +1
@josepht
Contributor

josepht commented Aug 22, 2016

I've updated @elopio's two nits.
@josepht
Fix deprecation message.

All checks have passed

4 successful checks
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.04%) to 97.498%
Details
4ac4291
@josepht
Contributor

josepht commented Aug 22, 2016

retest this please
integration_tests/test_parser.py
+ exception = self.assertRaises(
+ subprocess.CalledProcessError,
+ subprocess.check_call, args, stderr=subprocess.DEVNULL,
+ stdout=subprocess.DEVNULL)
@kyrofa

kyrofa Aug 23, 2016

Member

Is this saved exception used anywhere?

@josepht

josepht Aug 23, 2016

Contributor

Nope, removed.

integration_tests/test_parser.py
+ exception = self.assertRaises(
+ subprocess.CalledProcessError,
+ subprocess.check_call, args, stderr=subprocess.DEVNULL,
+ stdout=subprocess.DEVNULL)
@kyrofa

kyrofa Aug 23, 2016

Member

Ditto.

@josepht

josepht Aug 23, 2016

Contributor

removed as well

@josepht
Remove unused variable.

All checks have passed

4 successful checks
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.04%) to 97.499%
Details
0d194c9
@kyrofa
Member

kyrofa commented Aug 23, 2016

I had a few nits, but you already fixed them during review, so while I confess a lack of familiarity with the parser that made the review somewhat superficial, 👍 from me.
@sergiusens
Collaborator

sergiusens commented Aug 23, 2016

👍 I will merge once the tests are done

@sergiusens sergiusens merged commit acc5bc9 into snapcore:master Aug 23, 2016
4 checks passed

4 checks passed

autopkgtest integration Success
Details
autopkgtest snaps Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 97.499%
Details

@josepht josepht deleted the josepht:bugs/1606933 branch Aug 23, 2016

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

@josepht @sergiusens
parser - Remove namespacing and subparts. (#705) 
- remove 'project-part' from wiki entries
- require 'parts' in wiki entries
- 'parts' should have what was the original 'project-part' plus any
  other parts

LP: #1606933

Signed-off-by: Joe Talbott <joe.talbott@ubuntu.com>
b607297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment