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

Multiple dirs class set #400

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

Conversation

hgraca
Copy link
Contributor

@hgraca hgraca commented Jul 27, 2023

Currently, if we want to include two folders from the project root, we need to add the root of the project and then exclude everything we dont want to include, and remember to add a folder to the config if we add a new folder to the project root.

With the MultipleDirClassSet we can specify just the directories we want it to scan,
removing all cruft from our config.

@jdreesen
Copy link

How about refactoring ClassSet so that it can handle multiple directories instead?

The constructor is private so we could change it (without BC break) and add a second static constructor fromDirs(string ...$directoryList). It shouldn't be a breaking change, right?

@hgraca
Copy link
Contributor Author

hgraca commented Jul 28, 2023

Yes! That was my first approach, but I found some reason to do it differently... Dont remember what it was though. :(

We can even change the named constructor without a BC break.

my only concern here is with the getDir() method, cozz its return will not be atomic anymore, but on the other hand we are only using it for writing it on the screen.

Anyway, as I can't see anymore what drove me to this approach, I will simplify as you suggest.

@hgraca hgraca force-pushed the feat/multiple_dirs_class_set branch from a9b0366 to 501feda Compare July 28, 2023 18:45
@hgraca
Copy link
Contributor Author

hgraca commented Jul 28, 2023

@jdreesen done, and force pushed

@hgraca hgraca force-pushed the feat/multiple_dirs_class_set branch from 501feda to e1a979b Compare July 29, 2023 09:19
@hgraca hgraca force-pushed the feat/multiple_dirs_class_set branch from e1a979b to f27b65a Compare August 5, 2023 20:50
@codecov-commenter
Copy link

Codecov Report

Merging #400 (f27b65a) into main (976c200) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##               main     #400   +/-   ##
=========================================
  Coverage     94.33%   94.33%           
  Complexity      571      571           
=========================================
  Files            67       67           
  Lines          1500     1500           
=========================================
  Hits           1415     1415           
  Misses           85       85           
Files Changed Coverage Δ
src/ClassSet.php 100.00% <100.00%> (ø)

@AlessandroMinoccheri
Copy link
Member

@hgraca Before approving, can you please add something to the readme file :) ?

@hgraca hgraca force-pushed the feat/multiple_dirs_class_set branch from f27b65a to 990416a Compare September 5, 2023 07:45
This way we can specify just the directories we want it to scan,
as opposed to include all dirs in the root and then
excluding all dirs we don't want in the set.
@hgraca hgraca force-pushed the feat/multiple_dirs_class_set branch from 990416a to 7aa145e Compare September 5, 2023 07:49
@hgraca
Copy link
Contributor Author

hgraca commented Sep 5, 2023

@AlessandroMinoccheri done

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.

5 participants