Converge all commands into one #433

Merged
merged 11 commits into from Apr 7, 2016

Conversation

Projects
None yet
2 participants
Collaborator

sergiusens commented Apr 7, 2016

LP: #1567058
LP: #1567041

Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com

sergiusens added some commits Apr 7, 2016

Converge all commands into one
LP: #1567058

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>
Collaborator

sergiusens commented Apr 7, 2016

I am also fixing bug #1567041 here

sergiusens added some commits Apr 7, 2016

from snapcraft import common
+from snapcraft import lifecycle # noqa
+from snapcraft import plugins # noqa
@kyrofa

kyrofa Apr 7, 2016

Member

Why would these issue warnings?

@sergiusens

sergiusens Apr 7, 2016

Collaborator

Because they are imported but not used, but by doing this you can just import snapcraft and get snapcraft.plugins and snapcraft.lifecycle already imported as well 😄

Just like you get os.path when importing os

@kyrofa

kyrofa Apr 7, 2016

Member

Thanks for explaining!

@@ -115,7 +115,11 @@
import os
import shutil
+from snapcraft._help import topic_help # noqa
+from snapcraft._store import login, logout, upload # noqa
@kyrofa

kyrofa Apr 7, 2016

Member

Elsewhere we use a format like:

from snapcraft._store import (
    login,
    logout,
    upload
)

Should we be consistent?

@sergiusens

sergiusens Apr 7, 2016

Collaborator

Those are imported but not used, so I'd like to call them out explicitly.
It's defacto done like this for a package __init__.py

@kyrofa

kyrofa Apr 7, 2016

Member

Ah, which answers my other question there, too 😃 .

@sergiusens

sergiusens Apr 7, 2016

Collaborator

I can give you a personal note here too, I don't like the way tupling looks either 😛

@kyrofa

kyrofa Apr 7, 2016

Member

Let's say you imported 15 things here. How do you break the line?

@sergiusens

sergiusens Apr 7, 2016

Collaborator

With tupling, but it would also be an indication that we are doing something wrong IMHO

snapcraft/lifecycle.py
+ f.write(yaml)
+ logger.info('Created snapcraft.yaml.')
+
+
def execute(step, part_names=None):
"""Exectute until step in the lifecycle.
@kyrofa

kyrofa Apr 7, 2016

Member

Just noticed this says "Exectute." Might as well fix, eh?

@sergiusens

sergiusens Apr 7, 2016

Collaborator

with what? "Executes"? Sorry I keep getting lost with these.

@kyrofa

kyrofa Apr 7, 2016

Member

Just remove the extra T: s/Exectute/Execute/

snapcraft/main.py
+ this.
+ -s <step>, --step <step> only clean the specified step and those
+ that depend upon it. <step> can be one
+ of: pull, build, stage or strip.
@kyrofa

kyrofa Apr 7, 2016

Member

Still a relative docopt newbie, but should --step be here if it's in the Usage section above? Doesn't it make it a global option, when it should really be specific to clean?

@sergiusens

sergiusens Apr 7, 2016

Collaborator

It doesn't really matter. I can break down the header (Options) into "Options for cleaning", "Options for building"
and then just Options.

Meaning of [options] shortcut slightly changed. Previously it meant "any known option". Now it means "any option not in usage-pattern". This avoids the situation when an option is allowed to be repeated unintentionally.

from the first lines of https://github.com/docopt/docopt

snapcraft/main.py
+ snapcraft [options] stage [<part> ...]
+ snapcraft [options] strip [<part> ...]
+ snapcraft [options] clean [<part> ...] [--step <step>]
+ snapcraft [options] snap [<directory> --output <snap-file>]
@kyrofa

kyrofa Apr 7, 2016

Member

Does this syntax mean that --output is required when specifying <directory>, and not possible to specify when not specifying <directory>? Should this instead be snapcraft [options] snap [<directory>] [--output <snap-file>] ?

@sergiusens

sergiusens Apr 7, 2016

Collaborator

yeah, probably (and also to the snapcraft with no sub-command)

@sergiusens

sergiusens Apr 7, 2016

Collaborator

It is totally valid POSIX to do it this way, you can look at the last bullet here:
https://github.com/docopt/docopt#option-descriptions-format

Also, the tests pass and we have tests for -o and . If you think it would look nicer to put them in individual brackets that's fine, but look at git pull:

SYNOPSIS
    git pull [options] [<repository> [<refspec>...]]
snapcraft/main.py
+ that depend upon it. <step> can be one
+ of: pull, build, stage or strip.
+ -o <snap-file>, --output <snap-file> used in case you want to rename the
+ snap.
@kyrofa

kyrofa Apr 7, 2016

Member

Same question about the global option versus step-specific options.

snapcraft/main.py
snapcraft [options]
+ snapcraft [options] init
+ snapcraft [options] pull [<part> ...]
+ snapcraft [options] build [<part> ...]
@kyrofa

kyrofa Apr 7, 2016

Member

Now that these are all together, should --no-parallel-build be an option specific to build?

sergiusens added some commits Apr 7, 2016

Merge branch 'bugfix/1567058/merge-commands' of github.com:sergiusens…
…/snapcraft into bugfix/1567058/merge-commands
snapcraft/main.py
+ elif args['help']:
+ snapcraft.topic_help(args['<topic>'] or args['<plugin>'],
+ args['--devel'], args['topics'])
+ else: # snap of default:
@kyrofa

kyrofa Apr 7, 2016

Member

s/snap of default/snap by default/

@sergiusens

sergiusens Apr 7, 2016

Collaborator

I wanted to say "or" but "by" is better :-)

El 07/04/16 a las 12:58, Kyle Fazzari escribió:

In snapcraft/main.py
ubuntu-core#433 (comment):

+def run(args):

  • lifecycle_command = _get_lifecycle_command(args)
  • argless_command = _get_command_from_arg(args)
  • if lifecycle_command:
  •    snapcraft.lifecycle.execute(lifecycle_command, args['<part>'])
    
  • elif argless_command:
  •    argless_command()
    
  • elif args['clean']:
  •    snapcraft.lifecycle.clean(args['<part>'], args['--step'])
    
  • elif args['upload']:
  •    snapcraft.upload(args['<snap-file>'])
    
  • elif args['help']:
  •    snapcraft.topic_help(args['<topic>'] or args['<plugin>'],
    
  •                         args['--devel'], args['topics'])
    
  • else: # snap of default:

s/snap of default/snap by default/


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/ubuntu-core/snapcraft/pull/433/files/71398bd40679e08f4a3233187e516894d69e7242#r58897251

+ snapcraft [options] stage [<part> ...]
+ snapcraft [options] strip [<part> ...]
+ snapcraft [options] clean [<part> ...] [--step <step>]
+ snapcraft [options] snap [<directory> --output <snap-file>]
@kyrofa

kyrofa Apr 7, 2016

Member

Still got this.

@sergiusens

sergiusens Apr 7, 2016

Collaborator

It is totally valid POSIX to do it this way, you can look at the last bullet here:
https://github.com/docopt/docopt#option-descriptions-format

Also, the tests pass and we have tests for -o and . If you think it would look nicer to put them in individual brackets that's fine, but look at git pull:

SYNOPSIS
    git pull [options] [<repository> [<refspec>...]]
@kyrofa

kyrofa Apr 7, 2016

Member

Ah, then I'm good with this. My intuition misled me 😃 .

Member

kyrofa commented Apr 7, 2016

I'm a little sad to see the commands go, but I understand the need. 👍

Collaborator

sergiusens commented Apr 7, 2016

El 07/04/16 a las 13:24, Kyle Fazzari escribió:

I'm a little sad to see the commands go, but I understand the need. 👍

I feel the same. But it was an invitation to a problem as those commands aren't necessarily independent.

@sergiusens sergiusens merged commit ef54d29 into snapcore:master Apr 7, 2016

4 checks passed

Examples tests Success
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 95.666%
Details

kyrofa added a commit that referenced this pull request Apr 7, 2016

Converge all commands into one (#433)
LP: #1567058
LP: #1567041

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>

@sergiusens sergiusens deleted the sergiusens:bugfix/1567058/merge-commands branch Aug 30, 2016

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

Converge all commands into one (#433)
LP: #1567058
LP: #1567041

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment