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

feat: implement reversed-filter-group-priority option #702

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

Conversation

Pavel-Sulimau
Copy link
Contributor

@Pavel-Sulimau Pavel-Sulimau commented Apr 18, 2024

Description

This PR adds a new filtering option that implements the problem/desired behavior described in this issue.

The implementation is backward compatible, so the default filtering behavior should stay as it is.

Ideally, I'd love to have the possibility to prioritize filters based on the order of their input, but it seems to me that would require way more changes, so this is the simpler trade-off solution I have come up with so far.


The alternative approach I ended up implementing in the CI workflow of our big project implies setting the global MELOS_PACKAGES scope filter with the values from melos list --diff=origin/main...HEAD --include-dependents. The approach is described in https://itnext.io/flutter-selective-ci-checks-2d79ffbd26e5 article. Hopefully, it is helpful for other looking for optimization in their CI pipelines!

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
  • ! -- Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 refactor -- Code refactor
  • ci -- Build configuration change
  • 📝 docs -- Documentation
  • 🗑️ chore -- Chore

@Pavel-Sulimau Pavel-Sulimau force-pushed the feature/reversed-filter-group-priority branch from c5ec3d2 to aa01c11 Compare April 18, 2024 21:44
@Pavel-Sulimau Pavel-Sulimau force-pushed the feature/reversed-filter-group-priority branch from aa01c11 to eb854ee Compare April 18, 2024 21:51
@Pavel-Sulimau Pavel-Sulimau marked this pull request as ready for review April 18, 2024 21:59
@Pavel-Sulimau
Copy link
Contributor Author

Hey @Salakar, @spydon, your feedback on the proposed changes would be greatly appreciated. Thank you in advance!

@spydon
Copy link
Collaborator

spydon commented May 21, 2024

Hey @Salakar, @spydon, your feedback on the proposed changes would be greatly appreciated. Thank you in advance!

I'm still in the US, but I should have time to look at this on the weekend again! I'm still a bit unsure of the reverse concept and priority groups.

@spydon
Copy link
Collaborator

spydon commented May 27, 2024

I had a look again and I agree with what it is trying to do, but as I've said before I don't like the complexity of introducing the priority groups. I haven't come up with any other ideas though, do you have any other ideas how this could be handled?

@Pavel-Sulimau
Copy link
Contributor Author

@spydon, the most obvious and prominent idea in my opinion is to implement filters' prioritization based on the order of their input, however, I foresee this feature will require quite some changes.

Alternatively, we could just create a flag (how would we call it?) specifically handling this case to prioritize applyDiff and applyIncludeDependentsOrDependencies, but it looks like a less generic solution.

So, I agree that the priority groups is not the greatest way to solve the case, but I don't see many other ways at the moment how to do that elegantly without much effort/potential to break what is already working.

@spydon
Copy link
Collaborator

spydon commented May 29, 2024

@spydon, the most obvious and prominent idea in my opinion is to implement filters' prioritization based on the order of their input, however, I foresee this feature will require quite some changes.

That sounds like a great idea! If some filters aren't coming in through flags, but through config, we have to have a way to decide the order between the flags and those ones too right?

@Pavel-Sulimau
Copy link
Contributor Author

If some filters aren't coming in through flags, but through config, we have to have a way to decide the order between the flags and those ones too right?

I believe it should be possible.

@spydon
Copy link
Collaborator

spydon commented May 31, 2024

If some filters aren't coming in through flags, but through config, we have to have a way to decide the order between the flags and those ones too right?

I believe it should be possible.

Do you want to dig deeper into that option? I think it would be a better solution to the problem.

@Pavel-Sulimau
Copy link
Contributor Author

Pavel-Sulimau commented May 31, 2024

I would love to, however as I noted in the PR description it will likely require way more work for which I do not have enough time in the upcoming month or 2, so I'll leave it like this in case someone has time and desire to implement the other approach, otherwise I can get back to it myself sometime later.

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