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

Added words from Laravel Framework and Horizon #116

Merged
merged 3 commits into from
Jan 19, 2025

Conversation

andrey-helldar
Copy link
Contributor

@andrey-helldar andrey-helldar commented Jan 15, 2025

What:

  • Bug Fix
  • New Feature

Description:

Laravel Framework

  • apa (Str::apa())
  • doesnt (doesntExist method of Builder or Collection)
  • dont ($dontReport) property name or method name from exceptions
  • incrementing (models)
  • ulid

Laravel Horizon

  • cooldown (balanceCooldown)

Base

  • oauth
  • ttl

@calvinludwig
Copy link
Contributor

I believe that instead of adding contractions, like "doesnt" or "dont", to the whitelist, it should just not report it as a misspelling.

We can do this by matching the checked word to each Aspell suggestions; if the contraction's apostrophe is the only difference, ignore it as a misspelling.

What do you guys think @nunomaduro and @c0nst4ntin ?

@julio-cavallari
Copy link
Contributor

julio-cavallari commented Jan 16, 2025

I believe that instead of adding contractions, like "doesnt" or "dont", to the whitelist, it should just not report it as a misspelling.

We can do this by matching the checked word to each Aspell suggestions; if the contraction's apostrophe is the only difference, ignore it as a misspelling.

What do you guys think @nunomaduro and @c0nst4ntin ?

I think it's a fair thought, dont or doesnt in a class, method, property, function, etc., really shouldn't be a misspelling

But when it comes to descriptive texts like doc blocks, texts in templates, etc., this is a misspelling, 'cause dont theoretically doesn't exist and shouldn't be used to document something in the code, or be shown in the content of a website/system/app.

So I don't know if we should really ignore it whenever a case like this is suggested as misspelling.

@andrey-helldar
Copy link
Contributor Author

Or, if possible, add a check for the type that if it is the name of a property, method, function, class, or other control value, then don't consider it an error, but if the word occurs in text, such as a docblock, then return an error.

But how correct this will be outside Laravel is a big question.

@c0nst4ntin
Copy link
Collaborator

c0nst4ntin commented Jan 18, 2025

@andrey-helldar @julio-cavallari I agree, in theory we should not mark words like "dont" as a spelling mistake in code names. But at the same time, we should detect it in doc blocks.

The only question is, how can we achieve this? We would need to keep context about what type of text we are checking.

However, this is a more challenging topic. Maybe you can open an issue for this and we can challenge this in a new pull request.

For now maybe take them out of this pull request. The other words look fine to me 👍🏼

Will mark this as a draft. Please mark it back as "ready for review" once you changed it.

@c0nst4ntin c0nst4ntin added the presets Changes the presets label Jan 18, 2025
@c0nst4ntin c0nst4ntin marked this pull request as draft January 18, 2025 13:26
@andrey-helldar
Copy link
Contributor Author

@c0nst4ntin, @julio-cavallari, I agree.

I cleaned up the words and also created an issue: #120

@andrey-helldar andrey-helldar marked this pull request as ready for review January 18, 2025 14:39
@c0nst4ntin c0nst4ntin merged commit e9a09d2 into peckphp:main Jan 19, 2025
18 checks passed
@andrey-helldar andrey-helldar deleted the patch-1 branch January 19, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presets Changes the presets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants