New command to view next [x] posts

[yesyayen]

#55

[yesyayen]

I added a new variable 'top_story' in .haxornewsconfig file.
Initially the test suits calls self.load_config_top_count() function in config.py, that time this new variable is not yet added to the file .haxornewsconfig
What should I do to avoid this error, to make sure this new variable is added to the file before calling load on it.
'NoOptionError: No option 'top_story' in section: 'haxor-news''

[codecov-io]

Current coverage is 93.73%

Merging #67 into master will increase coverage by 0.06%

@@             master        #67   diff @@
==========================================
  Files            32         32          
  Lines          1484       1532    +48   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1390       1436    +46   
- Misses           94         96     +2   
  Partials          0          0          

Powered by Codecov. Last updated by 7c2d85a...d67d9ca

[yesyayen]

nvm. fixed it.

[donnemartin]

Thanks for the PR! I'll try to review it this week.

[donnemartin]

Hi, thanks again for putting in the PR.

'NoOptionError: No option 'top_story' in section: 'haxor-news''

I'm also getting this error, which I think existing users would get, but not new users after d6446af.

What should I do to avoid this error, to make sure this new variable is added to the file before calling load on it.

Hmm, I'll have to think some more about how to best handle this for newly added configs.

Regarding the behavior of the new next command: As a user, I think I would expect next to fetch the next set of posts based on the previous command I just issued.

The example in #53 used hn top 10, but if I do:

 $ hn show 20 
 $ hn next

I feel the desired behavior is to show the next 20 show posts. Currently it seems to show posts 11-20 from top.

[yesyayen]

I did not consider the show command for next. I just considered top for next command.
Your point makes more sense. To do that we should also store the latest command the user executed.

$ hn top 10
   .haxornewsconfig.story_count = 10 and .haxornewsconfig.latest_cmd = top
$ hn next          #shows 'top' post 10-20
   .haxornewsconfig.story_count = 20 and .haxornewsconfig.latest_cmd = top
$ hn show 21
   .haxornewsconfig.story_count = 21 and .haxornewsconfig.latest_cmd = show
$ hn next 11     #shows 'show' post 22-32
   .haxornewsconfig.story_count = 32 and .haxornewsconfig.latest_cmd = show

I feel the desired behavior is to show the next 20 show posts. Currently it seems to show posts 11-20 from top.

I have set the default to 10 for next. So it will show the next 10 show posts.
Or should I use the count the user used with top or show and make it as default value for show unless specified?

[yesyayen]

Updated the code

[donnemartin]

Thanks for the updates! I'll try to review them this week.

[donnemartin]

@yesyayen sorry haven't been able to review this yet, it's on my short term TODO list.

[donnemartin]

Just a quick note, this is still on my radar. Really busy lately--I hope to get to it soon.

[donnemartin]

Hi @yesyayen, thanks for making the revisions. Sorry for the late review, been tied up lately.

I did some more testing, below are some thoughts.

I did not consider the show command for next. I just considered top for next command.

I was thinking next would work for all the commands haxor-news supports for listing articles, sorry I didn't mean to imply that only show should be additionally supported, I just used it as an example :)

I have set the default to 10 for next. So it will show the next 10 show posts.
Or should I use the count the user used with top or show and make it as default value for show unless specified?

 $ hn top 20   # See 20 posts
 $ hn next     # See only the next 10 posts

I think the desired behavior is to show the next 20 posts.

I'm also getting this error, which I think existing users would get, but not new users.

CONFIG_LATEST_CMD and CONFIG_STORIES_SEEN_COUNT result in configparser.NoOptionError exceptions on startup for existing users, I think at a minimum we should try/catch these and set them to default values if the options aren't present. We might want to create a method to wrap parser.getint and parser.get to reduce duplicate logic.

Just a heads up, I'm going to be quite busy over the next few months, so that might impact my ability to provide timely feedback if you'd like to submit subsequent revisions.

Thanks!