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.

Sign up for GitHub

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

Jump to bottom

Create type definition #19

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Create type definition #19

wants to merge 5 commits into from

Conversation

Sytten
Copy link

@Sytten Sytten commented May 6, 2020

Fixes #18

@Sytten
Copy link
Author

Sytten commented May 6, 2020

@smusali if you have time to review

IgorHalfeld
IgorHalfeld previously approved these changes May 11, 2020
View reviewed changes
@smusali smusali requested review from smusali and jakedipity May 11, 2020 15:45
@smusali smusali added bug Something isn't working enhancement New feature or request labels May 11, 2020
@Sytten
Copy link
Author

Sytten commented May 18, 2020

Just a little ping to get some feedback, thanks!

@Sytten
Copy link
Author

Sytten commented May 24, 2020 •
edited
Loading

Another ping just to make sure I am not forgotten
@smusali @jakedipity

smusali
smusali suggested changes May 25, 2020
View reviewed changes
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @Sytten, for your work! We appreciate it a lot! And sorry for the late response since we have been having some other high prioritized tasks!

I have added some comments/requests to change for keeping it consistent with this.

index.d.ts Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@Sytten
Copy link
Author

Sytten commented May 25, 2020

@smusali Let me know if you prefer that I copy the props from the parent, otherwsie feel free to merge.

smusali
smusali suggested changes Jun 6, 2020
View reviewed changes
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

Let's have a complete conversion and then merge!

index.d.ts Outdated Show resolved Hide resolved
@smusali smusali self-requested a review June 6, 2020 19:10
smusali
smusali suggested changes Jun 6, 2020
View reviewed changes
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

Dear @Sytten, what we would like to see is the consistency with this.

@Sytten Sytten requested a review from smusali June 6, 2020 23:33
@Sytten
Copy link
Author

Sytten commented Jun 6, 2020

Dear @smusali, I hope you will the changes to you liking. Please see my comment above concerning a potential bug with defaults. I am also starting to consider that maybe the options interface should not inherit from the parents since it's particular mix of both (the case of level is problematic for example).

@smusali
Copy link
Contributor

smusali commented Jun 10, 2020

Dear @smusali, I hope you will the changes to you liking. Please see my comment above concerning a potential bug with defaults. I am also starting to consider that maybe the options interface should not inherit from the parents since it's particular mix of both (the case of level is problematic for example).

That's fine! I'll have a couple of requests as well

smusali
smusali suggested changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

Well, in order to keep consistency, I'm trying to compare it with our Node.js Code Library to which the types have already been added and most probably, after publishing the new version of logdna-winston including this addition, I'll do the same for our Bunyan Support as well. So, a few things I wanna request:

  • Please, specify the types inside package.json similar to this
  • This captures Logger module's methods as well; so, here we can see LogDNATransport has a log method but it looks like it's inherited from Transport and it's already defined here; so, there is no need to capture it separately, right? Can you confirm it, please?

Thanks!

index.d.ts
interface TransportOptions
extends Transport.TransportStreamOptions,
ConstructorOptions {
/** The LogDNA API key. */
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like there is some kinda indentation issue here - can you fix?

@Sytten
Copy link
Author

Sytten commented Jul 6, 2020

@smusali I think it would be best if you take over, I do not have the bandwidth to go back and forth over this issue anymore. Thanks!

@smusali smusali self-requested a review July 16, 2021 12:06
@TrejGun
Copy link

TrejGun commented Jul 28, 2021

@smusali can you please merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IgorHalfeld IgorHalfeld IgorHalfeld left review comments

@jakedipity jakedipity Awaiting requested review from jakedipity

@smusali smusali Awaiting requested review from smusali

At least 2 approving reviews are required to merge this pull request.

Assignees

@Sytten Sytten

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add types
4 participants
@Sytten @smusali @TrejGun @IgorHalfeld