Skip to content

Clean-up and documentation - #71

Merged
KodrAus merged 3 commits into
datalust:devfrom
nblumhardt:doc-coverage
Sep 13, 2019
Merged

Clean-up and documentation#71
KodrAus merged 3 commits into
datalust:devfrom
nblumhardt:doc-coverage

Conversation

@nblumhardt

Copy link
Copy Markdown
Member

This began with an effort to document more of the API of this library, but in doing so I found a few places (mostly historical warts) where the API could be improved.

There are some more changes here, particularly around Link, which has been simplified quite a bit - it's now always a URI template (and a "literal" URI is represented as a template with no parameters). Some unused constructors were removed - we do use these elsewhere in the Seq codebase, but updating usages and moving the object -> Dictionary parameter handling to Seq itself if necessary should be straightforward.

SeqApiClient.HttpClient now always includes the API key and (version-identifying) Accept header; we always added these for requests made through the library, but requests made directly against the client would not include them (diminishing the value of exposing HttpClient in the first place).

Needs some bake-time on dev :-)

…s authenticated requests with correct Accept headers
…some historical issues with Link, removing the confusing isLiteral mechanism to cement the expectation that all links in the Seq API are encoded as URI templates, even if they contain zero parameters
@nblumhardt

Copy link
Copy Markdown
Member Author

Needs some bake-time on dev :-)

Actually, since 5.1.1 already includes quite a bit of churn, might be better to push it through with that. I'll do more testing via the integration harness before #70 goes out the door.

public string GetUri(string name)
{
return this[name].GetUri();
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is trivially replaced with its body, with less magic.

namespace Seq.Api.Serialization
{
public class LinkCollectionConverter : JsonConverter
class LinkCollectionConverter : JsonConverter

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the converter is only used with LinkCollection, and it's plugged in via an attribute, there's no use case for this being public.

Comment thread src/Seq.Api/Model/Link.cs
/// Construct a link.
/// </summary>
/// <param name="template">The URI template.</param>
public Link(string template)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A link is now strictly a template; the previous isLiteral flag would skip template execution, but this also caused validation to be skipped - GetUri() on an accidentally-flagged isLiteral link wouldn't return a URI but instead a template.

Switching LinkCollectionConverter to use Template directly avoids the redundant template processing during serialization that isLiteral previously provided.

Comment thread src/Seq.Api/Model/Link.cs
/// </summary>
public static Link Empty { get; } = default;

public Link(string href, IDictionary<string, object> parameters)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-accepting overloads just conflated templates with their instantiated URIs; parameters are now applied in GetUri() only.

@KodrAus KodrAus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@KodrAus
KodrAus merged commit 03ad65c into datalust:dev Sep 13, 2019
@nblumhardt nblumhardt mentioned this pull request Sep 13, 2019
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

Successfully merging this pull request may close these issues.

2 participants