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

Inject project dependencies #71

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Inject project dependencies #71

merged 1 commit into from
Nov 9, 2023

Conversation

eygraber
Copy link
Contributor

@eygraber eygraber commented Nov 8, 2023

Stacked on #70

Copy link
Collaborator

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

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

I guess we can merge this after rebasing on #70

  - Helps to not use Project so there isn't an accidental issue with configuration cache
@eygraber
Copy link
Contributor Author

eygraber commented Nov 9, 2023

Rebased

@loosebazooka
Copy link
Collaborator

There doesn't seem to be any problem to merging this, but looking at this now, I'm not sure what this solves?

@eygraber
Copy link
Contributor Author

eygraber commented Nov 9, 2023

It technically doesn't solve anything since #72 ended up moving the ARQ out of the task execution. If it didn't then project couldn't be used to get the dependencies since usage of project is disallowed during task execution if configuration cache is enabled.

This PR pulls the dependencies out of Project instead of passing Project around to highlight the issue, and serve as an example of why not passing Project around is a good idea.

It also technically is good practice from a dependency perspective because these classes don't need Project; they need the classes that Project provides instances of.

@loosebazooka loosebazooka merged commit c5be3c2 into spdx:main Nov 9, 2023
1 check passed
@eygraber eygraber deleted the di branch November 9, 2023 17:27
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