Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add gathering data from github #1
Conversation
elopio
requested changes
Dec 5, 2017
Nice work! Thanks a lot.
I would like to keep in this repo only individual scripts, to leave people to get them and combine them to do their own stuff. So please remove the main, and leave it only on your repo. Also the init changes that you did to simplify the imports. Nothing wrong with them, just a personal preference of what to keep in this repo.
There is one thing I don't like about this. You are passing a dict that gets updated. That's called an output parameter, and sometimes it could be useful, but in general it should be avoided. The code is clearer, and simpler, if you just have one return value, and the caller takes care of using that value to update a variable, or something else.
Thank you!
| +### Requirements: | ||
| + * Python 2.7 | ||
| + * Urlparse | ||
| + * Requests |
elopio
Dec 5, 2017
Owner
This is just a collection of scripts that will be copied somewhere else. This README is great for your snapgatherer project, but it's not very useful here. What would be great here is to include the requirements on the header of each .py file. Nice job with this.
| @@ -0,0 +1 @@ | ||
| +from github.snapcraft import * |
elopio
Dec 5, 2017
Owner
Please remove this from this pull request. Again, it's great for the tool you made from the scripts, not for this repo with just a bunch of scripts.
| @@ -1,37 +1,122 @@ | ||
| -#!/usr/bin/env python2 | ||
| +#!/usr/bin/python2 |
elopio
Dec 5, 2017
Owner
Please revert this change. /usr/bin/env will support a python2 interpreter not installed in /usr/bin, like it happens with snaps.
| - count[plugin] = 1 | ||
| - snapcraft_yaml_file.close() | ||
| - return count | ||
| +_scriplets = ["prepare", "build", "install", "version-script"] |
elopio
Dec 5, 2017
Owner
This is a constant, so it should be _SCRIPTLETS.
For consistency, please use only single quotes for strings, instead of double quotes.
| -def _get_snapcraft_yaml_file(github_repo): | ||
| +def get_snapcraft_yaml_file(github_repo_url): | ||
| + """ | ||
| + Extracts snapcraft file from reposutory |
| + """ | ||
| + Extracts snapcraft file from reposutory | ||
| + :param github_repo_url: string, url of the repository | ||
| + :return: |
elopio
Dec 5, 2017
Owner
Thanks for adding the comments. But you left that return incomplete, please document also the value returned..
| - return urllib2.urlopen(url) | ||
| - except: | ||
| - pass | ||
| + resp = requests.request("GET", url) |
elopio
Dec 5, 2017
Owner
I prefer to use full words, because abbreviations are complex for non-native english speakers, like me. Please replace resp with response.
| + if resp.status_code == 200: | ||
| + return resp.text | ||
| + | ||
| +def get_plugins(snapcraft_file, plugins=None): |
| +def _get_source_type_from_uri(source): | ||
| + """ | ||
| + Detect source type of source | ||
| + Shamelesly stolen from https://github.com/snapcore/snapcraft/tree/master/snapcraft |
elopio
Dec 5, 2017
Owner
It's not stealing, it's called free software :D
These scripts have a gpl license, which is the same that snapcraft uses. So it would be nice to have a better comment here.
Something like: Code copied from the snapcraft project: https://github.com/snapcore/snapcraft
| - | ||
| -cache_dir = os.path.join( |
| +scriplets = dict() | ||
| +sources = dict() | ||
| + | ||
| +def updates_dicts(sfile): |
| + | ||
| +def updates_dicts(sfile): | ||
| + """ | ||
| + Updates your snapfile dictianories |
DeniskaMazur
commented
Dec 5, 2017
|
Understood, will fix it. |
DeniskaMazur
commented
Dec 5, 2017
|
Updated code, squashed commits, waiting for your review :) |
| @@ -1,37 +1,126 @@ | ||
| -#!/usr/bin/env python2 | ||
| +#!/usr/bin/env/python2 |
elopio
Dec 6, 2017
Owner
This is a bogus change. You shouldn't modify it, so please change the / back to a space.
elopio
Dec 6, 2017
Owner
oh, wait, actually this python files has no main, so this shebang is not doing anything here.
You can just remove the line.
| import urlparse | ||
| +import re |
elopio
Dec 6, 2017
Owner
It's common to sort the python imports alphabetically, grouping them by type: first python libs, then external libs, then libs local to the project. So, this should be:
import re
import urlparse
import requests
| - count[plugin] = 1 | ||
| - snapcraft_yaml_file.close() | ||
| - return count | ||
| +_SCRIPLETS = ["prepare", "build", "install", "version-script"] |
elopio
requested changes
Dec 6, 2017
I've left a few minor comments, mostly about style because I value consistency and clarity above all. But this is very good, I will approve your code-in task while you make the minor changes. Thanks!
| - return urllib2.urlopen(url) | ||
| - except: | ||
| - pass | ||
| + response = requests.request("GET", url) |
| + | ||
| + if response.status_code == 200: | ||
| + return response.text | ||
| + |
elopio
Dec 6, 2017
Owner
The spacing should be consistent. Top-level functions should be separated by two empty lines.
| + if response.status_code == 200: | ||
| + return response.text | ||
| + | ||
| +def get_plugins_count(snapcraft_file, plugins=None): |
| + else: | ||
| + plugins[plugin] = 1 | ||
| + return plugins | ||
| + |
| + plugins[plugin] = 1 | ||
| + return plugins | ||
| + | ||
| +def get_scriplets_count(snapcraft_file, scriplets=None): |
| + else: | ||
| + scriplets[line] += 1 | ||
| + return scriplets | ||
| + |
| + scriplets[line] += 1 | ||
| + return scriplets | ||
| + | ||
| +def get_sources_count(snapcraft_file, sources=None): |
| + else: | ||
| + sources[src_type] = 1 | ||
| + return sources | ||
| + |
DeniskaMazur
commented
Dec 6, 2017
|
updated, squashed |
DeniskaMazur
commented
Dec 6, 2017
•
Edited 1 time
-
DeniskaMazur
Dec 6, 2017
-
Dec 6, 2017
|
Forgot to fix some stuff, now done |
DeniskaMazur commentedDec 5, 2017
No description provided.