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

Fix metaField/requestField/responseField types #259

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

davidgoli
Copy link
Contributor

Addresses #256

@lmeerma
Copy link

lmeerma commented Feb 17, 2021

When is this going to be merged?

@bithavoc
Copy link
Owner

the real changes we need to merge I believe are:

 metaField?: string | null;
    requestField?: string | null;
    responseField?: string | null;

I don't know about the rest. I can merge and release if you rebase with only those changes. We can open another issue to address the rest of the base interface refactoring.

Copy link
Owner

@bithavoc bithavoc left a comment

Choose a reason for hiding this comment

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

Please only include changes for metaField requestField and responseField.

@lmeerma
Copy link

lmeerma commented Feb 18, 2021

@bithavoc I agree, my problem would be fixed when only metafield, requestField and responseField are changed.

@davidgoli
Copy link
Contributor Author

@bithavoc Changes made, please take another look

@bithavoc
Copy link
Owner

Great, I agree there is some repetition, but we can fix that in another PR. I'm ready to merge, do you want to rebase to squash your commits in a single commit or you want github to do it?

extract shared base interface

make BaseErrorLoggerOptions extend BaseSharedLogger Options

Roll back type deduplication

revert object -> Record change
@davidgoli
Copy link
Contributor Author

@bithavoc done

@bithavoc bithavoc merged commit d7d1b3c into bithavoc:main Feb 18, 2021
@bithavoc
Copy link
Owner

Thank you.

@bithavoc
Copy link
Owner

Thank you 🙏 [email protected]

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.

3 participants