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

Add AfterEagerFind() #802

Merged
merged 1 commit into from
Jan 13, 2023
Merged

Add AfterEagerFind() #802

merged 1 commit into from
Jan 13, 2023

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Jan 2, 2023

See #557
Closes #476

See #557
Closes #476

Signed-off-by: aeneasr <[email protected]>
@sio4
Copy link
Member

sio4 commented Jan 6, 2023

Not directly related to the topic of this PR, but I would like to postpone all new feature requests, especially adding new APIs until we clean up the current issues and solidify the design structure.

There were, still are, a bunch of open issues and recently I worked on them to fix and/or categorize them. (see #770 and #773) During that time, I found that especially the code for the association and eager feature has no clear design direction and that could be one reason for the issue. I made some milestones for the clean-up, the sequence of them are:

  • v6.x.0 - columns, keys, and datatypes (currently 12 open issues)
  • v6.y.0 - better SQL support (currently 2 open issues)
  • v6.z.0 - association improvement (currently 34 open issues)

Can we work on the backlog first along with the structural/design hardening before adding new features? I guess we could add new features related to the existing issues at some point in the plan/milestone when we can make sure of the solid fundamental design. (I think this could be a good approach since many of the issues happened or couldn't be closed early because we have a kind of... loose design concept. Doing this also will help us to make a better decision of direction on any particular issue)

What do you think? Could you please share your idea?

CC: @gobuffalo/core-managers

@aeneasr
Copy link
Member Author

aeneasr commented Jan 6, 2023

Hey, totally understand that. We have struggled quite a bit with the relationship code ourselves. Too much reflection, and side effects. However, AfterFind is pretty much useless for any models that have associations. The related issues are open for over 3 years and it’s a very simple yet important change. Therefore, I strongly recommend to merge it. The changed code is minimal, and the pattern already exists. Alternatively we can also break BC and move AfterFind to have the associations loaded. Not fixing the problem for another year and seeing whether the work around the refactoring manifests will not work for us at least.

@sio4
Copy link
Member

sio4 commented Jan 10, 2023

Thanks for your opinion! I will take a look at the PR and your explanation as soon as possible!

@aeneasr
Copy link
Member Author

aeneasr commented Jan 10, 2023

Appreciated :)

Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

Hi @aeneasr, sorry for the delayed review. Overall, it looks good to me!

I have two tiny questions and added comments. I think the PR is already OK to be merged, but I would like to hear your thought about them. (If you agree but have no time to fix them, I will merge it as-is, and will fix them in a separate PR. Just let me know)

if err := x.AfterEagerFind(c); err != nil {
return err
}
}
Copy link
Member

@sio4 sio4 Jan 12, 2023

Choose a reason for hiding this comment

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

It could be tiny. Since those two blocks are exclusive to each other, I would prefer to rewrite the block for readability as below. What do you think? (same for blocks in line 49)

	if eager {
		if x, ok := m.Value.(AfterFindable); ok {
			if err := x.AfterFind(c); err != nil {
				return err
			}
		}
	} else {
		if x, ok := m.Value.(AfterEagerFindable); ok {
			if err := x.AfterEagerFind(c); err != nil {
				return err
			}
		}
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically just a habit of keeping nesting flat - would be fine either way :)

})

if err != nil {
return err
}

if q.eager {
err = q.eagerAssociations(model)
err := q.eagerAssociations(model)
Copy link
Member

Choose a reason for hiding this comment

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

Just curious if there is a specific reason for adding ':' here. If there is a specific reason, an inline comment could be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's mostly a coding habit on my end, also fine either way

@sio4 sio4 self-assigned this Jan 12, 2023
@sio4 sio4 added f: associations the associations feature in pop enhancement New feature or request labels Jan 12, 2023
@sio4 sio4 added this to the v6.1.1 milestone Jan 12, 2023
@sio4 sio4 merged commit e2fe45b into main Jan 13, 2023
@sio4 sio4 deleted the add-eager-after-hook branch January 13, 2023 13:24
@aeneasr
Copy link
Member Author

aeneasr commented Jan 13, 2023

Thank you for the review and merge @sio4 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request f: associations the associations feature in pop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define AfterFind behavior for Eager connections
2 participants