Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Collapse identifiers seperated by hyphens or underscores if they exist in the file dictionary #199

Merged
merged 11 commits into from
Oct 4, 2024

Conversation

grantlemons
Copy link
Contributor

No description provided.

@grantlemons grantlemons self-assigned this Oct 3, 2024
@grantlemons
Copy link
Contributor Author

Tests need to be fixed.

@grantlemons grantlemons linked an issue Oct 3, 2024 that may be closed by this pull request
@grantlemons grantlemons marked this pull request as ready for review October 3, 2024 23:30
@@ -242,6 +242,11 @@ impl TokenKind {
matches!(self, TokenKind::Punctuation(Punctuation::At))
}

pub fn is_case_separator(&self) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably be two separate functions, but it's probably OK for now. If someone really wants it I can figure out how to do OR in pattern matching.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably fine to leave this here, but if you do, add a doc-comment that explains what a "case separator" is and why it's needed.

I would much rather create an OR Pattern and use that though, since we need one anyway. It's up to you which you decide to do (laziness is totally fine in this case).

Copy link
Contributor

@lukasmwerner lukasmwerner left a comment

Choose a reason for hiding this comment

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

Works great in harper-ls. Just a small issue for harper-cli

harper-core/src/parsers/cases.rs Outdated Show resolved Hide resolved
@elijah-potter
Copy link
Collaborator

Overall, this is really great work @grantlemons. I've left a few (nitpicky) comments on some bits. They should be pretty straightforward changes.

Other than those, would you mind changing your commit messages to match Conventional Commits? Feel free to force-push to the feature branch.

I would also mention issue #160 in them:

fix(#160): yadda yadda yadda

@@ -242,6 +242,11 @@ impl TokenKind {
matches!(self, TokenKind::Punctuation(Punctuation::At))
}

pub fn is_case_separator(&self) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably fine to leave this here, but if you do, add a doc-comment that explains what a "case separator" is and why it's needed.

I would much rather create an OR Pattern and use that though, since we need one anyway. It's up to you which you decide to do (laziness is totally fine in this case).

harper-core/src/parsers/cases.rs Outdated Show resolved Hide resolved
harper-core/src/parsers/cases.rs Outdated Show resolved Hide resolved
pub use markdown::Markdown;
pub use mask::Mask;
pub use plain_english::PlainEnglish;

pub use crate::token::{Token, TokenKind, TokenStringExt};

#[cfg(not(feature = "concurrent"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised this wasn't already here. How was harper-cli able to compile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk, I just checked, and it turns out mask::Masker uses Sync too and doesn't have a concurrent cfg.

harper-core/src/parsers/cases.rs Outdated Show resolved Hide resolved
@grantlemons grantlemons merged commit 40ade1e into master Oct 4, 2024
7 checks passed
@grantlemons grantlemons deleted the collapse_seps branch October 4, 2024 17:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: allow 'misspellings' that are identifiers
3 participants