Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeline - missing 'AddedToProject', 'RemovedFromProject' and more events #1594

Closed
karelz opened this issue Apr 27, 2017 · 11 comments
Closed

Comments

@karelz
Copy link

karelz commented Apr 27, 2017

Looks like variant of #1563 - which doesn't seem to be fixed by looking at source code: https:/octokit/octokit.net/blob/master/Octokit/Models/Response/EventInfo.cs#L74

Got it for dotnet/roslyn issues (numbers: 3 and 5).

I tried 0.24.1-alpha NuGet, which doesn't have even #1563 fix (I hit also LineCommented and CommitCommented). Will try latest master build later ...

System.AggregateException: One or more errors occurred. ---> System.ArgumentException: Requested value 'addedtoproject' was not found.
   at System.Enum.EnumResult.SetFailure(ParseFailureKind failure, String failureMessageID, Object failureMessageFormatArgument)
   at System.Enum.TryParseEnum(Type enumType, String value, Boolean ignoreCase, EnumResult& parseResult)
   at System.Enum.Parse(Type enumType, String value, Boolean ignoreCase)
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 124
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 1478
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 165
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 1505
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 165
   at Octokit.SimpleJson.DeserializeObject(String json, Type type, IJsonSerializerStrategy jsonSerializerStrategy) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 583
   at Octokit.SimpleJson.DeserializeObject[T](String json, IJsonSerializerStrategy jsonSerializerStrategy) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 595
   at Octokit.Internal.SimpleJsonSerializer.Deserialize[T](String json) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 21
   at Octokit.Internal.JsonHttpPipeline.DeserializeResponse[T](IResponse response) in D:\\repos\\octokit.net\\Octokit\\Http\\JsonHttpPipeline.cs:line 62
   at Octokit.Connection.<Run>d__54`1.MoveNext() in D:\\repos\\octokit.net\\Octokit\\Http\\Connection.cs:line 574
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Octokit.Internal.ReadOnlyPagedCollection`1.<GetNextPage>d__3.MoveNext() in D:\\repos\\octokit.net\\Octokit\\Http\\ReadOnlyPagedCollection.cs:line 0
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Octokit.ApiPagination.<GetAllPages>d__0`1.MoveNext() in D:\\repos\\octokit.net\\Octokit\\Clients\\ApiPagination.cs:line 0
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.Task`1.get_Result()
   at Repository.LoadIssueTimelines(IEnumerable`1 issueNumbers) in C:\\git\\GitHubIssues_Timeline\\BugReport\\Repository.cs:line 289
---> (Inner Exception #0) System.ArgumentException: Requested value 'addedtoproject' was not found.
   at System.Enum.EnumResult.SetFailure(ParseFailureKind failure, String failureMessageID, Object failureMessageFormatArgument)
   at System.Enum.TryParseEnum(Type enumType, String value, Boolean ignoreCase, EnumResult& parseResult)
   at System.Enum.Parse(Type enumType, String value, Boolean ignoreCase)
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 124
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 1478
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 165
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 1505
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 165
   at Octokit.SimpleJson.DeserializeObject(String json, Type type, IJsonSerializerStrategy jsonSerializerStrategy) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 583
   at Octokit.SimpleJson.DeserializeObject[T](String json, IJsonSerializerStrategy jsonSerializerStrategy) in D:\\repos\\octokit.net\\Octokit\\SimpleJson.cs:line 595
   at Octokit.Internal.SimpleJsonSerializer.Deserialize[T](String json) in D:\\repos\\octokit.net\\Octokit\\Http\\SimpleJsonSerializer.cs:line 21
   at Octokit.Internal.JsonHttpPipeline.DeserializeResponse[T](IResponse response) in D:\\repos\\octokit.net\\Octokit\\Http\\JsonHttpPipeline.cs:line 62
   at Octokit.Connection.<Run>d__54`1.MoveNext() in D:\\repos\\octokit.net\\Octokit\\Http\\Connection.cs:line 574
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Octokit.Internal.ReadOnlyPagedCollection`1.<GetNextPage>d__3.MoveNext() in D:\\repos\\octokit.net\\Octokit\\Http\\ReadOnlyPagedCollection.cs:line 0
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Octokit.ApiPagination.<GetAllPages>d__0`1.MoveNext() in D:\\repos\\octokit.net\\Octokit\\Clients\\ApiPagination.cs:line 0<---
@khellang
Copy link
Contributor

khellang commented Apr 27, 2017

What about making EventInfo.Event some kinda union type between string and EventInfoState. It would just be a simple wrapper around a string, but would allow you to call something like TryParse(out EventInfoState) to get an enum version. That way, if the event type is new (missing from the enum), you could still handle it as a string, and it won't blow up when new types are added.

The type could also have an implicit conversion to EventInfoState that would continue to throw if the value is invalid. This would make it (more or less) backwards compatible.

@karelz
Copy link
Author

karelz commented Apr 27, 2017

And RemovedFromProject event -- dotnet/roslyn issue 35, after local fix to address AddedToProject ...

I like your idea to not hardcoding all events, make the list more resilient ... but of course I know near 0 about Octokit.net design ;)

@karelz karelz changed the title Timeline - missing 'AddedToProject' event Timeline - missing 'AddedToProject' and 'RemovedFromProject' events Apr 27, 2017
@shiftkey
Copy link
Member

Yeah, #1591 should fix these. But given how often new events are introduced I really think deserializing to string with more flexibility for callers to then map it to something is a more sane long-term strategy for this area 🤔 .

@karelz karelz changed the title Timeline - missing 'AddedToProject' and 'RemovedFromProject' events Timeline - missing 'AddedToProject', 'RemovedFromProject' and more events Apr 27, 2017
@karelz
Copy link
Author

karelz commented Apr 27, 2017

Thanks for the link to the PR. I just hit 'MovedColumnsInProject' ... I will grab the list from the PR ;)

@ryangribble
Copy link
Contributor

We had a sort of proof of concept of a way to handle this in #1504 which essentially would see all response enums contain an extra member (eg OctokitUnsupportedValue) that could be used when encountering an unknown value, whilst also exposing a string xxxxxRaw property containing the unparsed api return value. But it didn't necessarily feel quite right so I hadn't pushed it too hard. I do quite like the elegance of @khellang's suggestion above

@khellang
Copy link
Contributor

khellang commented Apr 27, 2017

Had to run out for lunch, but I may or may not have a PR brewing, using something like

[DebuggerDisplay("{DebuggerDisplay,nq}")]
[SuppressMessage("Microsoft.Naming", "CA1711:IdentifiersShouldNotHaveIncorrectSuffix")]
public struct StringEnum<TEnum> : IEquatable<StringEnum<TEnum>>
    where TEnum : struct
{
    private readonly string _value;

    private TEnum? _parsedValue;

    public StringEnum(TEnum parsedValue)
        : this(parsedValue.ToString())
    {
        _parsedValue = parsedValue;
    }

    public StringEnum(string value)
    {
        _value = value;
        _parsedValue = null;
    }

    public string Value
    {
        get { return _value; }
    }

    public TEnum ParsedValue
    {
        get { return _parsedValue ?? (_parsedValue = ParseValue()).Value; }
    }

    internal string DebuggerDisplay
    {
        get { return Value; }
    }

    public bool TryParse(out TEnum value)
    {
        if (string.IsNullOrEmpty(Value))
        {
            value = default(TEnum);
            return false;
        }

        return Enum.TryParse(Value, ignoreCase: true, result: out value);
    }

    public bool Equals(StringEnum<TEnum> other)
    {
        return string.Equals(Value, other.Value, StringComparison.OrdinalIgnoreCase);
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj))
        {
            return false;
        }

        return obj is StringEnum<TEnum> && Equals((StringEnum<TEnum>) obj);
    }

    public override int GetHashCode()
    {
        return Value != null ? StringComparer.OrdinalIgnoreCase.GetHashCode(Value) : 0;
    }

    public static bool operator ==(StringEnum<TEnum> left, StringEnum<TEnum> right)
    {
        return left.Equals(right);
    }

    public static bool operator !=(StringEnum<TEnum> left, StringEnum<TEnum> right)
    {
        return !left.Equals(right);
    }

    public static implicit operator StringEnum<TEnum>(string value)
    {
        return new StringEnum<TEnum>(value);
    }

    public static implicit operator StringEnum<TEnum>(TEnum parsedValue)
    {
        return new StringEnum<TEnum>(parsedValue);
    }

    public override string ToString()
    {
        return Value ?? "null";
    }

    private TEnum ParseValue()
    {
        TEnum value;
        if (TryParse(out value))
        {
            return value;
        }

        throw new ArgumentException(string.Format(
            CultureInfo.InvariantCulture,
            "Value '{0}' is not a valid '{1}' enum value.",
            ToString(),
            typeof(TEnum).Name));
    }
}

It's basically a drop-in replacement of the existing enum, with no additional code changes necessary because of the implicit conversion.

@ryangribble
Copy link
Contributor

Even better 👍

@ryangribble
Copy link
Contributor

Closing this since the specific missing properties is resolved in #1591 and the ongoing improvements to handle Enums in a more resilient fashion is in #1595

@karelz
Copy link
Author

karelz commented Apr 29, 2017

Great, thanks!

Out of curiosity: What is cadence for your NuGet alpha releases? Last nuget.org alpha build is from January. I'd like to know when I can stop using the project-to-project reference. Not a high pri ... but would be nice.

If there is anything I can help with to release new alpha version, please let me know. If it is not over my head or super-costly, I will be happy to help.

@ryangribble
Copy link
Contributor

So we don't currently regularly release alpha packages to nuget... that one release was actually a special case to expose our dotnetcore port to a wider audience

You can add the appveyor CI feed to your nuget settings and pull packages from there if you need

My plan is once we've merged the dotnetcore and CAKE build script (should be soon) we'll look at automating pre release packages pushed to nuget on each PR merged to master

@karelz
Copy link
Author

karelz commented Apr 29, 2017

ok, I'll use the AppVeyor CI feed (I think I already added it anyway, then I realized it is not fixed either there yet ;-) - I just forgot about it ...)
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants