Skip to content

refactor: split words by language#8

Merged
JonPurvis merged 7 commits intopestphp:masterfrom
davideprevosto:feature/split-words-by-language
Aug 16, 2024
Merged

refactor: split words by language#8
JonPurvis merged 7 commits intopestphp:masterfrom
davideprevosto:feature/split-words-by-language

Conversation

@davideprevosto
Copy link
Copy Markdown
Contributor

This Pull Request splits words by language. It also adds a first version of the Italian stop-words configuration.

@JonPurvis
Copy link
Copy Markdown
Collaborator

JonPurvis commented Aug 7, 2024

Hey @davideprevosto

Would you be able to take a look at the failing static analysis checks? From what I can see:

  1. composer lint needs running
  2. You've moved the config files into Config/words/ but when including them, not looking within /words.

Thanks!

P.S as @faissaloux suggested, could you rename the words directory to profanities too please

@faissaloux
Copy link
Copy Markdown
Contributor

@JonPurvis I think the words files should live in a separated directory in Config to separate them from tolerated file like @davideprevosto did this directory may have multiple files later on for each language, I'm just thinking to rename the dir words to profanities 🤔

@JonPurvis
Copy link
Copy Markdown
Collaborator

Ah yes good point @faissaloux, somehow I forgot we had tolerated.php now 😆 (have updated my comment)

Agreed on a directory name change, but how about lang?

@faissaloux
Copy link
Copy Markdown
Contributor

Agreed on a directory name change, but how about lang?

Emmmmm lang doesn't represent what's inside. I think profanities is better!

If someone gets there will know exactly what en.php is for exactly from its directory name (profanities)

@JonPurvis
Copy link
Copy Markdown
Collaborator

profanities it is 😄

Copy link
Copy Markdown
Contributor

@faissaloux faissaloux left a comment

Choose a reason for hiding this comment

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

Can we please make it more flexible to accept more languages by just adding more languages files on the profanities directory without adding languages files to the code one by one.

Copy link
Copy Markdown
Contributor

@faissaloux faissaloux left a comment

Choose a reason for hiding this comment

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

Can you please add this note SHOULD ALL WORDS BE lowercase! to all words files on line 5

@JonPurvis
Copy link
Copy Markdown
Collaborator

Hey @davideprevosto 👋

I'm keen to get this work into the package. Would you have any time soon to action the required changes? If not, I'd be happy to commit to this branch to finish the work off and get it merged 😄

@davideprevosto
Copy link
Copy Markdown
Contributor Author

Hi @JonPurvis, I tried to follow your instructions and I pushed a few changes. I run composer lint and I suppose Laravel Pint did its job.

I also run composer test but I run into the following issue, I suppose I need to further investigate what I am doing wrong.

composer test
> rector --dry-run
sh: /Users/xxx/Library/Application: No such file or directory
Script rector --dry-run handling the test:refacto event returned with error code 127
Script @test:refacto was called via test

@JonPurvis
Copy link
Copy Markdown
Collaborator

Hey @davideprevosto

Not actually sure what could be causing that error. Do you get the same thing when you run composer test:refacto?

@davideprevosto
Copy link
Copy Markdown
Contributor Author

composer test:refacto

Yes @JonPurvis, I am getting the same error

composer test:refacto
> rector --dry-run
sh: /Users/xxxx/Library/Application: No such file or directory
Script rector --dry-run handling the test:refacto event returned with error code 127

@JonPurvis
Copy link
Copy Markdown
Collaborator

Okay, can you try running composer test:lint? I want to see if all of them have the same issue or it's just refacto

@davideprevosto
Copy link
Copy Markdown
Contributor Author

Okay, can you try running composer test:lint? I want to see if all of them have the same issue or it's just refacto

image

@JonPurvis
Copy link
Copy Markdown
Collaborator

Hmm, that's odd. It works for me even with your changes. Although it does fix one thing:

    FileLineFinder::where(function (string $line) use (&$foundWords): bool {
        return str_contains(strtolower($line), strtolower(array_values($foundWords ?? [])[0]));
    })

to

    FileLineFinder::where(function (string $line) use (&$foundWords): bool {
        return str_contains(strtolower($line), strtolower((string) array_values($foundWords ?? [])[0]));
    })

If you make that change, and then amend the merge conflict, this branch should be good to merge. I'm not sure why refacto doesn't run properly on your machine 😢

@JonPurvis
Copy link
Copy Markdown
Collaborator

Thanks @davideprevosto

Are you also able to sort the merge conflict please?

@JonPurvis JonPurvis merged commit eda1382 into pestphp:master Aug 16, 2024
@JonPurvis
Copy link
Copy Markdown
Collaborator

Thanks for the contribution @davideprevosto 🥳 I'll get a new release tagged later today!

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.

3 participants