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

feat: Topics #13

Merged
merged 11 commits into from
May 5, 2023
Merged

feat: Topics #13

merged 11 commits into from
May 5, 2023

Conversation

wh1337
Copy link
Collaborator

@wh1337 wh1337 commented May 4, 2023

No description provided.

@wh1337 wh1337 added the feature New feature or request label May 4, 2023
@wh1337 wh1337 self-assigned this May 4, 2023
wh1337 added 3 commits May 4, 2023 23:23
All methods are implemented as of this commit. There is still work to be done on proper error handling and documentation.
@wh1337 wh1337 requested a review from unicodeveloper May 5, 2023 05:55
@wh1337 wh1337 marked this pull request as ready for review May 5, 2023 05:55
NovuClient = novu;
}

public async void Dispose()
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why this function is empty?

Copy link
Collaborator Author

@wh1337 wh1337 May 5, 2023

Choose a reason for hiding this comment

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

When you implement the IDisposable interface on a text fixture, the method is required to be implemented as part of testing cleanup. However, we do not have anything to clean up after running this test. We have to implement IDisposable in order for xunit to pick up the fixture an use it as a dependency injection so we don't have to create new clients on each test and we can use the shared client through all tests in that class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay thanks for the clarification

Comment on lines +11 to +29
/// <example>
/// <para>
/// <code>
/// var topicRequest = new TopicCreateDto
/// {
/// Key = $"test:topic:{Guid.NewGuid().ToString()}",
/// Name = "Test Topic",
/// };
///
/// var topic = await client.Topic.CreateTopicAsync(topicRequest);
/// </code>
/// </para>
/// </example>
/// <param name="dto">
/// <see cref="TopicCreateDto"/>
/// </param>
/// <returns>
/// <see cref="TopicCreateResponseDto"/>
/// </returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why we have the docblocks like this in different html tags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These generate intellisense snippets like this:
image

It is also required for future docfx implementation for automatic documentation.

C# Comments: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/documentation-comments

Docfx: https://dotnet.github.io/docfx/

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have them in the other code, so I was wondering why we need them all of a sudden now.

Copy link
Contributor

@unicodeveloper unicodeveloper left a comment

Choose a reason for hiding this comment

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

Please can you update the readme as well?

@unicodeveloper unicodeveloper merged commit 9fdc7e3 into novuhq:main May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants