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

allow dev comments to be enabled from cli, opt in #138

Merged
merged 8 commits into from
Jan 16, 2021
Merged

Conversation

drahnr
Copy link
Owner

@drahnr drahnr commented Jan 15, 2021

What does this PR accomplish?

  • 🩹 Bug Fix
  • 🦚 Feature

Closes #132 .

Changes proposed by this PR:

A minor simplification attempt on cli args, and threads a bool to the places that need it.

📜 Checklist

  • Works on the ./demo sub directory
  • Test coverage is excellent and passes
  • Documentation is thorough

@drahnr drahnr added enhancement 🦚 New feature or request checker generic checker topics labels Jan 15, 2021
@drahnr drahnr requested a review from KuabeM January 15, 2021 13:20
@drahnr drahnr self-assigned this Jan 15, 2021
src/main.rs Show resolved Hide resolved
@@ -70,6 +69,19 @@ Options:
--skip-readme Do not attempt to process README.md files listed in Cargo.toml manifests.
"#;

#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, Deserialize)]
enum CheckerType {
#[serde(alias = "hunspell")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[serde(alias = "hunspell")]
#[serde(alias = "hunspell")]
#[serde(alias = "Hunspell")]

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's already named Hunspell, so that is already covered iiu the serde docs correctly.

Copy link
Collaborator

@KuabeM KuabeM left a comment

Choose a reason for hiding this comment

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

Passing the bool dev_comments around seems not too ergonomic, but it's the easiest way to integrate this fast and without too much refactoring 👍

@drahnr
Copy link
Owner Author

drahnr commented Jan 16, 2021

I agree it's not pretty, especially having it in traverse is just wrong from the logic partitioning. Needs a refactor, but I want to get this feature out the door with the next alpha, and refactor during the alpha / beta releases.

@drahnr drahnr merged commit d430b36 into master Jan 16, 2021
@drahnr drahnr deleted the bernhard-cli-args branch January 16, 2021 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checker generic checker topics enhancement 🦚 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make operations on dev comments opt-in
2 participants