Added enum ItemStateFilter to differentiate between search and list APIs #1140

prayankmathur · 2016-03-10T22:22:48Z

Fixes #1082
I have added an enum "ItemStateFilter"
and tried to explain it in the xml docs.
You can review it.

…ration tests.

ryangribble · 2016-03-13T03:34:51Z

so after having a look at the changes on the PR I have to say that I am backflipping a little on what i said in the issue #1082 !!

It looks like almost all usages of ItemState only allow values of Open or Closed. Eg issues, milestones and pull request objects, as well as the NewXXX and UpdateXXX Request classes for these. It doesn't really make sense to me, to use an enum called ItemStateFilter (that s described as being used for search filtering) on all of these objects and new/update message classes. It looks like pretty much the ONLY place that seems to allow All as an option to State is the List Issues/Milestones/Pull Requests type calls.

So Im wondering if perhaps we should:

Keep ItemState enum but flag the All value as [Deprecated] (to be removed in future)
Keep using this ItemState enum on all the existing places such as Issue Milestone and PullRequest objects and the NewXXX UpdateXXX request classes for them
Introduce the new ItemStateFilter enum, allowing open,closed,all and use it only on the `XXXRequest`` classes that are used for the "list" methods

That means less "overall" change, and the more sensible approach that ItemState is used for actual items and creating/updating them... whilst the ItemStateFilter is used when listing them

prayankmathur · 2016-03-13T09:53:29Z

@ryangribble
So basically, just reverse the names of the enum !! :P

ryangribble · 2016-03-13T10:24:29Z

Yep pretty much, and [Obsolete] the ItemState.All item

prayankmathur · 2016-03-13T12:02:33Z

@ryangribble
But then we come back to the same issue, that the user who is using octokit doesn't get all the issues on using ItemState.All

ryangribble · 2016-03-13T22:14:10Z

@ryangribble
But then we come back to the same issue, that the user who is using octokit doesn't get all the issues on using ItemState.All

I'm not sure I follow? The search API would no longer take ItemState as an option (it would be ItemStateFilter) so they wouldn't be able to specify it anymore?

prayankmathur · 2016-03-14T05:05:17Z

@ryangribble

I have made the changes. Take a look.

ryangribble · 2016-03-14T13:59:46Z

hi @prayankmathur, as with the other PR it would be great if you could give this one a nice title and description when you can! 😀

In terms of your latest changes, Im still seeing several uses of ItemStateFilter where I dont think they are valid.

To recap:
ItemStateFilter allows values Open, Closed, All and is used on operations that can accept all 3 values - namely the "List" calls under Issues, Milestones, PullRequest API clients
(eg client.Issues.GetAll(new IssueRequest(State: ItemStateFilter.All))

ItemState allows only Open and Closed (with "All" being marked [Deprecated], to be removed next release) and is used on operations where only Open or Closed are valid states.
The Search API client is the example that triggered this issue to be raised,
eg client.Search.SearchIssues(new SearchIssuesRequest(State: ItemState.Open))
however there are also several other use cases, such as updating an Issue/Milestone/PullRequest and creating a Milestone where the state can only be open or closed.

Your latest changes show IssueUpdate MilestoneUpdate NewMilestone and PullRequestUpdate classes as all using ItemStateFilter but shouldn't they be using ItemState since only open and closed are valid values for all of these?

…ing the necessary changes

prayankmathur · 2016-03-14T18:01:51Z

@ryangribble
Thanks for clearing that out. I got a little confused.
I have made the necessary changes.
Take a look.

And i hope you like the title. 😀

ryangribble · 2016-03-14T20:45:14Z

Octokit/Models/Response/Issue.cs

        /// </summary>
        public ItemState State { get; protected set; }

        /// <summary>
+        /// Whether the issue is open or close
+        /// </summary>
+        public ItemStateFilter FilteredState {get; protected set;}


This property shouldnt be needed

ryangribble · 2016-03-14T21:36:06Z

Looking really good!

Down to very nitpick type comments now... Ive flagged a few whitespace and XmlDoc tidy ups

The TravisCI build failure on linux was an environmental issue and not related to your changes... I've given it a kick

@shiftkey - im thinking since this is a breaking change anyhow (replacing ItemState with ItemStateFilter on the xxxSearch Request objects)... perhaps there is no need to Obselete the ItemState.All value and we can simply remove it as part of the breaking change. There shouldn't really be anyone using ItemState.All on Issue/PR/Milestone update/create anyway since it isn't a valid operation (not sure how the API handles it if requested actually!). Do you agree with not needing to Obsolete ItemState.All ?

ryangribble · 2016-03-14T21:40:50Z

Octokit/Models/Request/IssueRequest.cs

+        /// <summary>
+        /// All the issues. The option is Obsolete
+        /// </summary>
+        [Obsolete("The method is Obsolete. Dont specify the state in case of all queries")]


As per my other comment Im thinking we actually can just remove this guy and not need to Obsolete it, since we are making a breaking change anyway. Waiting for @shiftkey to confirm. If we DO need to obsolete it though, the warning message should be reworded, since this is not a "method". Something like

This value is obsolete and will be removed in a future version. For Request operations please use ItemStateFilter.All. For Search, Create and Update operations, "all" is not a valid option.

shiftkey · 2016-03-24T20:00:58Z

Octokit/Models/Request/IssueRequest.cs

@@ -121,23 +121,45 @@ public enum IssueFilter
    }

    /// <summary>
-    /// The range of states that an issue can be in.
+    /// Range of states for "Issues", "Milestones" and "Pull Request" API.


Don't worry about the quotation marks here

@shiftkey Removed the quotation marks. 😄

shiftkey · 2016-03-25T17:17:02Z

Octokit/Models/Response/Issue.cs

@@ -64,7 +64,7 @@ public Issue(Uri url, Uri htmlUrl, Uri commentsUrl, Uri eventsUrl, int number, I
        public int Number { get; protected set; }

        /// <summary>
-        /// Whether the issue is open or closed.
+        /// Whether the issue is "open", "closed" or "all".


If we're going to [Obsolete] All here, or just remove it, we should update the docs accordingly...

shiftkey · 2016-03-25T17:20:16Z

Just one more code comment to tidy up and I think this is good.

@ryangribble how does this read for the breaking changes documentation:

Decoupled request and response models usage of ItemState enum due to API rules. When querying for issues, milestones or pull requests these now use ItemStateFilter, and response models continue to use ItemState.

prayankmathur · 2016-03-25T18:19:09Z

@shiftkey
Which one ?
The one you mentioned in the diff ?

shiftkey · 2016-03-25T18:23:00Z

@prayankmathur correct, this one #1140 (comment)

prayankmathur · 2016-03-25T18:24:41Z

@shiftkey
Will remove the quotation marks right away.

prayankmathur · 2016-03-28T09:51:14Z

@shiftkey
Made the changes.Take a look.

shiftkey · 2016-03-28T17:03:02Z

@ryangribble over to you ✊

ryangribble · 2016-03-29T02:32:13Z

Octokit/Models/Request/IssueRequest.cs

        /// </summary>
+        [Obsolete("The value is Obsolete and will be removed in a future release as it is not a valid option for Search, Create and Update operations")]


As agreed by @shiftkey we can just DROP the All option here, no need to mark it [Obsolete] anymore

ryangribble · 2016-03-29T02:39:56Z

Hi guys, sorry for the delay I had a little break over the long weekend :)

It's looking great now @prayankmathur...
Ive marked up a couple of (final!) comment tweaks, and also noted that as per above, no need to [Obsolete] the ItemState.All option anymore - just 🔥 it

prayankmathur · 2016-03-29T12:00:54Z

@ryangribble
I have made the necessary changes. Take a look.

ryangribble · 2016-03-29T12:39:48Z

And with that, I think we are done with this one! Thanks for persisting through all the nitty gritty review 😆

prayankmathur · 2016-03-30T14:51:48Z

@ryangribble
The more I am able to improve octokit the better. 😀

shiftkey · 2016-04-08T18:00:42Z

release_notes: Add importers field to IMiscellaneousClient.Meta response

hnrkndrssn · 2016-06-30T21:48:49Z

@shiftkey it seems this fix didn't make it in to the Breaking Changes section of the release notes (I realised as it broke a couple of my scripts after I upgraded to latest), mind having a look and adding it in there?

shiftkey · 2016-07-01T05:50:05Z

@alfhenrik ugh, yep, missed this one.

Thanks for letting me know, I'll edit the GitHub release to include that change...

shiftkey · 2016-07-01T05:54:14Z

For reference, I added this comment to the GitHub release:

ItemStateFilter has been introduced to separate it from the search APIs, which still use ItemState. This affects IssueRequest.State, PullRequestRequest.State and MilestoneRequest.State. You'll need to rename any enum usages from ItemState to ItemStateFilter.

@alfhenrik can I get a 👍 that this matches with what you were seeing?

hnrkndrssn · 2016-07-01T06:09:38Z

I was using RepositoryIssueRequest but it inherits the State property from IssueRequest so all good.

prayankmathur added 5 commits March 10, 2016 21:23

Fixed Issue 1082 in code. Still need to fix the unit as well as integ…

7688d61

…ration tests.

Added the new enum ItemStateFilter to response/issue.cs

2dc6826

Added the new enum ItemStateFilter to response/issue.cs

39a810a

Issue 1082 fixed adding enum ItemStateFilter

d03e37b

Fixed issue 1082 adding enum ItemStateFilter

0f5d7c4

shiftkey added the bugfix label Mar 11, 2016

prayankmathur force-pushed the master branch from 3f7e0a1 to 0f5d7c4 Compare March 11, 2016 10:54

ryangribble mentioned this pull request Mar 13, 2016

SearchIssuesRequest with State = ItemState.All #1082

Closed

prayankmathur added 3 commits March 14, 2016 10:12

1140

9fd1c02

1140

546d7d5

Merge branch 'master' into master

56791f2

Updated the enum ItemState with deprecating the ItemState.All and mak…

397b1f1

…ing the necessary changes

prayankmathur changed the title ~~Fixed issue 1082~~ Added enum ItemStateFilter to differentiate between search and list APIs Mar 14, 2016

Added enum ItemStateFilter to differentiate between search and list APIs

786b8e1

ryangribble reviewed Mar 14, 2016
View reviewed changes

shiftkey reviewed Mar 24, 2016
View reviewed changes

Edited comment

da1485c

shiftkey reviewed Mar 25, 2016
View reviewed changes

Edited comment

d37774a

shiftkey assigned ryangribble Mar 28, 2016

ryangribble reviewed Mar 29, 2016
View reviewed changes

Removed all from ItemState and made some comment changes

c51bb07

ryangribble merged commit 6455001 into octokit:master Mar 29, 2016

ryangribble added the category: bug label Aug 18, 2017

nickfloyd added Type: Bug Something isn't working as documented and removed category: bug labels Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added enum ItemStateFilter to differentiate between search and list APIs #1140

Added enum ItemStateFilter to differentiate between search and list APIs #1140

prayankmathur commented Mar 10, 2016

ryangribble commented Mar 13, 2016

prayankmathur commented Mar 13, 2016

ryangribble commented Mar 13, 2016

prayankmathur commented Mar 13, 2016

ryangribble commented Mar 13, 2016

prayankmathur commented Mar 14, 2016

ryangribble commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

ryangribble Mar 14, 2016

ryangribble commented Mar 14, 2016

ryangribble Mar 14, 2016

shiftkey Mar 24, 2016

prayankmathur Mar 25, 2016

shiftkey Mar 25, 2016

shiftkey commented Mar 25, 2016

prayankmathur commented Mar 25, 2016

shiftkey commented Mar 25, 2016

prayankmathur commented Mar 25, 2016

prayankmathur commented Mar 28, 2016

shiftkey commented Mar 28, 2016

ryangribble Mar 29, 2016

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 29, 2016

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 30, 2016

shiftkey commented Apr 8, 2016

hnrkndrssn commented Jun 30, 2016

shiftkey commented Jul 1, 2016

shiftkey commented Jul 1, 2016

hnrkndrssn commented Jul 1, 2016

		/// </summary>
		[Obsolete("The value is Obsolete and will be removed in a future release as it is not a valid option for Search, Create and Update operations")]

Added enum ItemStateFilter to differentiate between search and list APIs #1140

Added enum ItemStateFilter to differentiate between search and list APIs #1140

Conversation

prayankmathur commented Mar 10, 2016

ryangribble commented Mar 13, 2016

prayankmathur commented Mar 13, 2016

ryangribble commented Mar 13, 2016

prayankmathur commented Mar 13, 2016

ryangribble commented Mar 13, 2016

prayankmathur commented Mar 14, 2016

ryangribble commented Mar 14, 2016

prayankmathur commented Mar 14, 2016

ryangribble Mar 14, 2016

Choose a reason for hiding this comment

ryangribble commented Mar 14, 2016

ryangribble Mar 14, 2016

Choose a reason for hiding this comment

shiftkey Mar 24, 2016

Choose a reason for hiding this comment

prayankmathur Mar 25, 2016

Choose a reason for hiding this comment

shiftkey Mar 25, 2016

Choose a reason for hiding this comment

shiftkey commented Mar 25, 2016

prayankmathur commented Mar 25, 2016

shiftkey commented Mar 25, 2016

prayankmathur commented Mar 25, 2016

prayankmathur commented Mar 28, 2016

shiftkey commented Mar 28, 2016

ryangribble Mar 29, 2016

Choose a reason for hiding this comment

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 29, 2016

ryangribble commented Mar 29, 2016

prayankmathur commented Mar 30, 2016

shiftkey commented Apr 8, 2016

hnrkndrssn commented Jun 30, 2016

shiftkey commented Jul 1, 2016

shiftkey commented Jul 1, 2016

hnrkndrssn commented Jul 1, 2016