Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Implement snapcraft search #608
Conversation
sergiusens
added some commits
Jun 25, 2016
elopio
reviewed
Jun 26, 2016
| + width = max_width | ||
| + if max_width: | ||
| + width = min(max_width, width) | ||
| + return width |
elopio
Jun 26, 2016
Member
This seems wrong.
You are calling: terminal_width = get_terminal_width(max_width=None). For not a tty, that will return None
Then you do: description_space = terminal_width - part_length - 5
So None - something - 5, will raise:
TypeError: unsupported operand type(s) for -: 'NoneType' and 'int'
elopio
reviewed
Jun 26, 2016
| @@ -117,6 +124,25 @@ def get_part(self, part_name, full=False): | ||
| remote_part.pop(key) | ||
| return remote_part | ||
| + def matches_for(self, part_match, max_len=0): | ||
| + sm = difflib.SequenceMatcher(isjunk=None, autojunk=False) | ||
| + sm.set_seq2(part_match) |
elopio
Jun 26, 2016
Member
using sm as a variable is a gopher thing. Something like "matcher" would be more pythonic.
elopio
reviewed
Jun 26, 2016
| + | ||
| + if add_part_name or add_description or part_match in part_name: | ||
| + matching_parts[part_name] = self._parts[part_name] | ||
| + matching_parts[part_name] = self._parts[part_name] |
elopio
reviewed
Jun 26, 2016
| @@ -117,6 +124,25 @@ def get_part(self, part_name, full=False): | ||
| remote_part.pop(key) | ||
| return remote_part | ||
| + def matches_for(self, part_match, max_len=0): |
sergiusens
Jun 26, 2016
Collaborator
The class is private, the method public, I don't see the reason for this one.
kyrofa
Jun 27, 2016
•
Edited 1 time
-
kyrofa
Jun 27, 2016
Member
-
Jun 27, 2016
This method is only ever used within this class though, no? Do you envision it being used by others? i.e. just because the class is private doesn't mean encapsulation is no longer important.
elopio
Jun 27, 2016
Member
This is how I see it: an internal class starts with _ and is used only in the module. An internal method starts with _ and is used only in the class. So internal classes can have internal methods. Of course, this is how I use internals in python, not a good practice I have seen everywhere. So we can have a discussion to define our rules for internals.
sergiusens
Jun 27, 2016
Collaborator
@elopio I am in agreement. An instance of _RemoteParts is created under the implementation of search and calls this method so it is not internal to the class https://github.com/ubuntu-core/snapcraft/pull/608/files#diff-ac0a8a66808d29ef0e343cb310093978R179
elopio
Jun 27, 2016
Member
hum, that is more complex. It's like a sandwich of internal. I'll say you win. :D
elopio
reviewed
Jun 26, 2016
| + if not matches: | ||
| + # apt search does not return error, we probably shouldn't either. | ||
| + logger.info('No matches found, try to run `snapcraft update` to ' | ||
| + 'refresh the remote parts cache.') |
elopio
Jun 26, 2016
Member
or maybe the part just doesn't exist. Should that be part of the message?
What about: to contribute new parts, read ?
elopio
reviewed
Jun 26, 2016
| + self.addCleanup(patcher.stop) | ||
| + | ||
| + @mock.patch('sys.stdout', new_callable=io.StringIO) | ||
| + def test_searching_for_a_part_that_exists(self, mock_stdout): |
elopio
Jun 26, 2016
Member
you are missing a test for a match in description, not in name.
And for completenes, maybe a match in name, not in description.
sergiusens
added some commits
Jun 26, 2016
|
@elopio ok, one more round! |
elopio
reviewed
Jun 27, 2016
| + matcher.set_seq1(part_name) | ||
| + add_part_name = matcher.ratio() >= _MATCH_RATIO | ||
| + | ||
| + if add_part_name or part_match in part_name: |
elopio
Jun 27, 2016
Member
Use parentheses here to make the rules of precedence clear:
if add_part_name or (part_match in part_name):
kyrofa
reviewed
Jun 27, 2016
| + for part_key in matches.keys(): | ||
| + description = matches[part_key]['description'] | ||
| + if len(description) > description_space: | ||
| + description = '{}...'.format(description[0:description_space]) |
kyrofa
Jun 27, 2016
Member
I haven't thought through the difficulty of this, but would it be better to wrap instead? Looking something like this:
$ snapcraft search qt
PART NAME DESCRIPTION
qt5conf This sets up qt5.conf for projects using qml
and other qt5 components that need an
Ubuntu standard internal path setup by
default like the Ubuntu Core Apps.
mqtt-paho/python2 mqtt-paho for python.
mqtt-paho mqtt-paho for python.
[...]
as opposed to:
$ snapcraft search qt
PART NAME DESCRIPTION
qt5conf This sets up qt5.conf for projects using...
mqtt-paho/python2 mqtt-paho for python.
mqtt-paho mqtt-paho for python.
[...]
kyrofa
Jun 27, 2016
Member
Filtering? You mean e.g. piping to grep? So then the way to see the entire description is just to snapcraft define it?
sergiusens
Jun 27, 2016
Collaborator
@kyrofa your idea is not bad, but a bug report would be better so if not grepping we present a different layout.
kyrofa
Jun 27, 2016
Member
Yeah no problem, I don't have strong feelings here. I can make a bug if you'd like one.
elopio
reviewed
Jun 27, 2016
| + 'source': 'http://source.tar.gz', | ||
| + 'description': 'this is a repetetive description ' * 3, | ||
| + 'maintainer': 'none', | ||
| + }, |
sergiusens
added some commits
Jun 27, 2016
|
There's a typo: repetetive instead of repetitive. Looks good to me. |
sergiusens
added some commits
Jun 27, 2016
|
|
sergiusens
merged commit ea6595e
into
snapcore:master
Jun 27, 2016
sergiusens
deleted the
sergiusens:bugfix/1596222/snapcraft-search
branch
Jun 27, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
sergiusens commentedJun 25, 2016
LP: #1596222
Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com