Let me laugh#3365
Conversation
elijah-potter
left a comment
elijah-potter
left a comment
There was a problem hiding this comment.
I appreciate the spirit of the rule, but I see that it's implementation is not as contained as I would prefer. Would you mind tweaking it to remove logic from LintGroup?
| const ALLOW_INFORMAL_LAUGHTER: &str = "AllowInformalLaughter"; | ||
| const SPELL_CHECK: &str = "SpellCheck"; | ||
| const SPLIT_WORDS: &str = "SplitWords"; |
There was a problem hiding this comment.
I try to avoid constants like this, especially if they simply stand alone and aren't clearly connected to any particular part of the code. Would you mind inlining them?
| let mut results = BTreeMap::new(); | ||
| let allow_informal_laughter = self.config.is_rule_enabled(ALLOW_INFORMAL_LAUGHTER); | ||
|
|
||
| // Normal linters | ||
| for (key, linter) in &mut self.linters { | ||
| if self.config.is_rule_enabled(key) { | ||
| results.insert(key.to_owned(), linter.lint(document)); | ||
| let mut lints = linter.lint(document); | ||
| if allow_informal_laughter && should_filter_informal_laughter_lints(key) { | ||
| lints.retain(|lint| !is_informal_laughter(lint.get_ch(document.get_source()))); | ||
| } | ||
| results.insert(key.to_owned(), lints); |
There was a problem hiding this comment.
As a rule, I try to avoid letting one linter affect the output of another, especially in a composition location like the LintGroup. It gives the struct more responsibility than it should be given.
Would it be possible to refactor this PR into a modification on the RepeatedWords rule, with some additions to the dictionary?
See:
harper-core/src/linting/repeated_words.rsharper-core/dictionary.dict
There was a problem hiding this comment.
Hmm, so would this mean it's no longer a distinct rule that a user could disable? For things like academic papers and such they won't want this to be enabled.
There was a problem hiding this comment.
I agree. A separate rule, maybe called SilenceLaughter could be added that stops laughter. It would be pretty straightforward to implement.
This way, each rule's responsibility is maintained, which makes it easier to audit each one down the line.
I'd be happy to make the changes for you.
elijah-potter
left a comment
elijah-potter
left a comment
There was a problem hiding this comment.
This looks good. Thanks!
|
This also reminds me of the |
3 participants
Summary
AllowInformalLaughterconfig rule.ha/hahlaughter chains likehah,haha, andhahahah.hah-style chains likehahhahlinted, and leaves nearby interjections likeah,ahah, andheheto existing behavior.This is, admittedly, a small rule with a big emotional range.
Tests
cargo test -p harper-core laughtercargo test -p harper-core default_configcargo test -p harper-core lint_descriptions_are_cleancargo run --bin harper-cli --release -- lint "hah"cargo run --bin harper-cli --release -- lint "I laughed hahahah."cargo run --bin harper-cli --release -- lint "hahhah"