Refactored the project #19

ayuhsya · 2016-12-06T17:34:59Z

Created two new python scripts:

core.py Contains the main WikiPage class with methods for each Wiki property.
helpers.py Contain methods to strip the JSON response.

And deleted the scripts wiki_cat.py, wiki_linkshere.py, wiki_images.py whose methods have been added in core.py script.

abinashmeher999 · 2016-12-06T21:17:20Z

@ayuhsya The PR looks great! 💯 Wonderfully arranged. I will review the PR properly tomorrow. Meanwhile @djr-jsr can you review the PR?

abinashmeher999 · 2016-12-07T21:54:05Z

sample/core.py

+        }
+
+    # Method to fetch categories
+    def get_categories(self, clshow="!hidden"):


One of the motivations of this project is so that the users need not bother themselves with the details of the API. Try to use generic names wherever possible. clshow is very misleading who isn't familiar with the API.

abinashmeher999 · 2016-12-07T21:59:01Z

sample/helpers.py

@@ -0,0 +1,19 @@
+import re


You need not create a separate file for helpers. Better keep them in the same file as core.py outside WikiPage class and prefix them with _. Names starting with _ are supposed to refer to functions that should not be messed with by the user unless the user is sure what they are doing. "We're all consenting adults here" 😉

abinashmeher999 · 2016-12-07T22:00:19Z

sample/core.py

+
+        img_list = helpers.beautiful(res, prop, 'File')
+        while 'continue' in res:
+            payload['imcontinue'] = res['continue']['imcontinue']


I think this should be self.payload.

abinashmeher999 · 2016-12-07T22:02:16Z

sample/helpers.py

+    return res['query']['pages'][pageids][prop]
+
+
+def beautiful(res, prop, strip_chars):


We need a better name for this 🤔

abinashmeher999 · 2016-12-07T22:03:02Z

sample/helpers.py

+    for pageid, page_content in res['query']['pages'].items():
+        if prop not in page_content:
+            continue
+        ret[pageid] = [strip_prop(content['title'], strip_chars) for content in page_content[prop]]


entry seems like a better replacement for content here.

@ayuhsya I meant [strip_prop(entry['title'], strip_chars) for entry in page_content[prop]]

@abinashmeher999 I've changed the variable name.

ayuhsya · 2016-12-08T09:59:43Z

@abinashmeher999 I've made the changes. I've deleted helpers.py but imo it will be required later when you'll see that core.py script is too long with all the methods for each API property. We'll be scrolling all the time to find where the helper methods are defined.

abinashmeher999 · 2016-12-08T10:23:16Z

sample/core.py

+    wk = WikiPage('843158','20715044')
+    pprint(wk.get_categories())
+    #pprint(wk.get_images())
+    #pprint(wk.get_linkshere())


Could you uncomment these 2? I need to check the output once just to be sure.

@abinashmeher999 Okay, done.

ayuhsya · 2016-12-08T10:37:20Z

@abinashmeher999 Every time I'm asked to make a change, I follow these commands to push them:

git add .
git commit -m "some message"
git rebase -i HEAD~2
git push origin +<name of branch>

I did rebase because you once asked me to squash all commits in one before making a PR.
Is this the right way or is there a better way?

abinashmeher999 · 2016-12-08T10:41:41Z

Squashing them is your choice. I asked them the last time because some of the commits had unclear names and some changes didn't need to have a commit of there own. In this case you could have kept separate commits. There's nothing like squashing all the commits in a PR is a good practice. I would advise that you fetch the upstream and rebase on top of the branch you are working on.

abinashmeher999 · 2016-12-08T10:42:35Z

Addresses #7

ayuhsya · 2016-12-08T10:44:20Z

And #15 too.
And maybe #3 too.

abinashmeher999 · 2016-12-08T10:47:53Z

@ayuhsya It doesn't address #3 fully. Maybe you can try that next. Very easy. Won't even take an hour.

abinashmeher999 · 2016-12-08T10:49:49Z

And of course the module name is not sample.

ayuhsya · 2016-12-08T10:52:58Z

@abinashmeher999 It says in the guide that the main module should reside in the sample folder.

abinashmeher999 · 2016-12-08T10:55:54Z

@ayuhsya 😐 That's because the project name is 'sample' https://github.com/kennethreitz/samplemod/blob/master/setup.py

ayuhsya · 2016-12-08T10:57:37Z

😢 Damn, I thought that was the standard.

abinashmeher999 · 2016-12-08T10:59:02Z

😆 We all have been there.

abinashmeher999 requested changes Dec 7, 2016

View reviewed changes

ayuhsya force-pushed the slave branch from d16c524 to 1629a9c Compare December 8, 2016 09:51

ayuhsya force-pushed the slave branch from 1629a9c to 9c777f5 Compare December 8, 2016 10:12

abinashmeher999 reviewed Dec 8, 2016

View reviewed changes

Refactored the code with suggested changes

959f686

ayuhsya force-pushed the slave branch from 9c777f5 to 959f686 Compare December 8, 2016 10:30

abinashmeher999 approved these changes Dec 8, 2016

View reviewed changes

abinashmeher999 merged commit 351f512 into abinashmeher999:master Dec 8, 2016

abinashmeher999 mentioned this pull request Dec 8, 2016

Refactor the code to make it easier to extend and add features later #7

Open

3 tasks

abinashmeher999 mentioned this pull request Dec 8, 2016

Optional parameter to decide whether to include hidden categories #15

Closed

ayuhsya deleted the slave branch December 8, 2016 10:51

ayuhsya restored the slave branch December 9, 2016 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored the project #19

Refactored the project #19

ayuhsya commented Dec 6, 2016 •

edited

Loading

abinashmeher999 commented Dec 6, 2016

abinashmeher999 Dec 7, 2016

abinashmeher999 Dec 7, 2016

abinashmeher999 Dec 7, 2016

abinashmeher999 Dec 7, 2016

abinashmeher999 Dec 7, 2016

abinashmeher999 Dec 8, 2016

ayuhsya Dec 8, 2016

ayuhsya Dec 8, 2016

ayuhsya commented Dec 8, 2016 •

edited

Loading

abinashmeher999 Dec 8, 2016

ayuhsya Dec 8, 2016

ayuhsya commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

ayuhsya commented Dec 8, 2016 •

edited

Loading

abinashmeher999 commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

ayuhsya commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

ayuhsya commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

		return res['query']['pages'][pageids][prop]


		def beautiful(res, prop, strip_chars):

Refactored the project #19

Refactored the project #19

Conversation

ayuhsya commented Dec 6, 2016 • edited Loading

abinashmeher999 commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayuhsya commented Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayuhsya commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

ayuhsya commented Dec 8, 2016 • edited Loading

abinashmeher999 commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

ayuhsya commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

ayuhsya commented Dec 8, 2016

abinashmeher999 commented Dec 8, 2016

ayuhsya commented Dec 6, 2016 •

edited

Loading

ayuhsya commented Dec 8, 2016 •

edited

Loading

ayuhsya commented Dec 8, 2016 •

edited

Loading