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

docs: add comparison of test functions and oauth provider table to contributing.md #1281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hoeseong19
Copy link

What kind of change does this PR introduce?

Docs update

What is the new behavior?

The comparison table is added to contributing.md

image

Additional context

There is a function (DisableSignupErrorWhen(Empty|No)Email) in the mix that tests for no(or empty) email.
It would be nice to have a choice between Empty and No.

@hoeseong19 hoeseong19 requested a review from a team as a code owner October 22, 2023 06:44
@kangmingtay
Copy link
Member

would prefer not to add this table since it would be an extra overhead to keep it up to date with the codebase

@hoeseong19
Copy link
Author

hoeseong19 commented Nov 6, 2023

Do you have any ideas of alternative to the table?
If it is decided that the comparison table will not be added, it will still be necessary to unify the names of functions.

@kangmingtay
Copy link
Member

kangmingtay commented Nov 7, 2023

@hoeseong19 probably better to have a go interface that defines the different cases to test a new oauth provider

@hoeseong19
Copy link
Author

If I do what you say, that would solve both of my problems!

I'll work on that feature and then make a new pull request.

@kangmingtay
Copy link
Member

@hoeseong19 what are the problems you are trying to solve here? it would be great if you can create an issue first to describe the problems you face and we can discuss the solution in there before creating a PR

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