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

Apply source scope by default in any loader #191

Open
wants to merge 4 commits into
base: 1.x
Choose a base branch
from

Conversation

hustlahusky
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #191 (55d7978) into master (fc7ead8) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #191      +/-   ##
============================================
- Coverage     94.48%   94.39%   -0.10%     
+ Complexity      937      936       -1     
============================================
  Files            81       81              
  Lines          2413     2409       -4     
============================================
- Hits           2280     2274       -6     
- Misses          133      135       +2     
Impacted Files Coverage Δ
src/Relation/Pivoted/PivotedPromise.php 89.36% <ø> (-0.23%) ⬇️
src/Select/JoinableLoader.php 87.64% <ø> (-2.36%) ⬇️
src/Select/RootLoader.php 100.00% <ø> (ø)
src/ORM.php 94.73% <100.00%> (-0.12%) ⬇️
src/Select/AbstractLoader.php 88.09% <100.00%> (+0.14%) ⬆️
src/Select/QueryBuilder.php 92.04% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc7ead8...55d7978. Read the comment docs.

@roxblnfk roxblnfk self-requested a review June 21, 2021 16:10
@wolfy-j
Copy link
Contributor

wolfy-j commented Jun 22, 2021

Please explain what usecase is covered by this change?

https://github.com/cycle/orm/blob/master/src/Factory.php#L177 Scopes were attached by default at source level - so all repositories were reading them correctly. Select was created without a scope on purpose.

@hustlahusky
Copy link
Contributor Author

Please explain what usecase is covered by this change?

I thought this more clear way to define scope on any loader, not just joinable. May be I get it wrong, but what is the purpose to not attach scope to select?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants