dopecodez / Wikipedia

Wikipedia for node and the browser
MIT License
82 stars 19 forks source link

Random page API #11

Closed friendofdog closed 3 years ago

friendofdog commented 3 years ago

closes #4

dopecodez commented 3 years ago

Other than the changes, the types and the tests look good. Don't worry about the failing coverage request, its a result as per the discussion in #12 , and we will take it up separately.

Once you've tested and feel like this can be merged to master, please go ahead and change the draft status.

You might have to merge the master branch though, I think it has moved ahead of your current branch.

friendofdog commented 3 years ago

Other than the changes, the types and the tests look good. Don't worry about the failing coverage request, its a result as per the discussion in #12 , and we will take it up separately.

Once you've tested and feel like this can be merged to master, please go ahead and change the draft status.

You might have to merge the master branch though, I think it has moved ahead of your current branch.

Got it.

Added a few tests for different return formats. Not sure how necessary this was, but I figure that the types I just added should be in tests somewhere.

dopecodez commented 3 years ago

The PR looks good @friendofdog . Thanks for the time you took to document all the types and the comprehensive testing too.

I went ahead and documented the new types and your new method so that we can release this feature as soon as this is merged.

I made a couple of changes in the flow as well. I added a default value as summary for our related method here. I also added an input type randomFormats to our method so TypeScript users can see those types available directly. I also added a test case for the same.

I also used your related object in one of the available related methods on wiki. So, thanks for that too 😄

We'll go ahead and merge if you feel like my changes work with your method

friendofdog commented 3 years ago

The PR looks good @friendofdog . Thanks for the time you took to document all the types and the comprehensive testing too.

I went ahead and documented the new types and your new method so that we can release this feature as soon as this is merged.

I made a couple of changes in the flow as well. I added a default value as summary for our related method here. I also added an input type randomFormats to our method so TypeScript users can see those types available directly. I also added a test case for the same.

I also used your related object in one of the available related methods on wiki. So, thanks for that too

We'll go ahead and merge if you feel like my changes work with your method

Just took a look at your changes. Everything makes sense (thanks for explaining things).

I'm not sure if they were missed or were intentionally left out, but I added mobileSections and html wherever they seemed to be missing.

dopecodez commented 3 years ago

The html part is a problem @friendofdog . It won't work because we are parsing the result and this will cause it to return an error.

The mobileSections part will work. Adding that makes sense but I think we need to remove the html part for now as having a random html might not be so useful. I don't feels like having a workaround for random in case of random html makes sense.

dopecodez commented 3 years ago

Thanks for the PR @friendofdog 🥇 . You can expect a release with this feature later today.