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

Merge context package into the main package #104

Merged
merged 2 commits into from
May 23, 2020
Merged

Conversation

sagikazarmark
Copy link
Collaborator

Fixes #96

This is a fully backwards compatible change by adding aliases for the necessary type (Context).

ToDos

  • NewContext does not have to be exported. Should it be?

Possible alternatives

  • Introduce an interface: that would be a somewhat backward incompatible change, but the Context should only be used by consumers as a type, it shouldn't be manually instantiated. (An interface can still be added after this PR)
  • Add a type alias to the context package: would be less code change, but eventually the context package should go away

@bkielbasa
Copy link
Collaborator

thanks for your contribution!

I think we can remove the context package in v2.0.

IMO we can make NewContext unexported.

@sagikazarmark
Copy link
Collaborator Author

Oh, it looks like we cannot make NewContext unexported, if we want to keep New in the context package.

@bkielbasa
Copy link
Collaborator

please fix linter issues and I can merge it

@sagikazarmark
Copy link
Collaborator Author

One of the failing lint rules seems to be false positive.

@sagikazarmark sagikazarmark force-pushed the merge-context branch 4 times, most recently from c1283fa to 0fac6be Compare May 23, 2020 10:14
@sagikazarmark
Copy link
Collaborator Author

gobdd.go:415:38: `t` can be `github.com/stretchr/testify/assert.TestingT` (interfacer)
func (def *stepDef) run(ctx Context, t TestingT, params [][]byte) {

I think this is false positive. I can either disable linting in this line with // nolint: interfacer, or disable the interfacer linter entirely.

@bkielbasa
Copy link
Collaborator

@sagikazarmark let's disable it for this line for now. If we'll have more problems like that we can disable it entirely.

@bkielbasa
Copy link
Collaborator

@sagikazarmark you don't have to rebase commits because I squash and merge them anyway :)

@bkielbasa bkielbasa merged commit ac8c48f into master May 23, 2020
@bkielbasa bkielbasa deleted the merge-context branch May 23, 2020 14:14
@bkielbasa
Copy link
Collaborator

Thanks for your contribution!

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.

Merge context package with the main package
2 participants