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

decoder: refactor SelfRefs use context #394

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

Conversation

giautm
Copy link
Contributor

@giautm giautm commented Jun 2, 2024

This PR refactors a mess in the codebase with allowSelfRefs passing around until it reaches the place it was used for.

The helper schema.WithActiveSelfRefs(ctx) is already defined by the author, so I just use it for this suppose; and remove the allowSelfRefs variable from the method.

Before:

type ReferenceOriginsExpression interface {
	ReferenceOrigins(ctx context.Context, allowSelfRefs bool) reference.Origins
}

After:

type ReferenceOriginsExpression interface {
	ReferenceOrigins(ctx context.Context) reference.Origins
}

@giautm giautm requested a review from a team as a code owner June 2, 2024 17:26
@giautm giautm changed the title decoder: refactor SelfRefs use context (#2) decoder: refactor SelfRefs use context Jun 2, 2024
@giautm
Copy link
Contributor Author

giautm commented Jun 4, 2024

@dbanck: please help to review

@jpogran
Copy link
Contributor

jpogran commented Jun 21, 2024

Hi @giautm, thank you for offering a contribution to this repo. Can you please put a description of your change in the PR, along with the reasoning for your change? Right now the PR description is empty and the commit message just points to giautm#2, which I did not find helpful either.

In general it is a good practice to open an issue describing the change you want so there is a chance for us to discuss the design before work is put into a PR that may not be accepted as is. Apologies, this repo does not have a CONTRIBUTING.md to have set that expectation, I will fix that.

Once we have an understanding of the intent of the change, we can review this PR.

@giautm
Copy link
Contributor Author

giautm commented Jun 23, 2024

@jpogran updated its description, thank you.

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