feat: add language option. #69
Conversation
WalkthroughThe recent changes introduce a Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/cli/src/commands/apply_pattern.rs (4 hunks)
Additional comments: 2
crates/cli/src/commands/apply_pattern.rs (2)
- 112-113: The addition of the
languagefield to theApplyPatternArgsstruct is well-implemented and aligns with the PR's objectives to enhance the CLI's usability.- 130-130: Initializing the
languagefield withDefault::default()in theDefaultimplementation forApplyPatternArgsis appropriate and ensures the correct default behavior.
There was a problem hiding this comment.
Thanks for contributing! Please provide a few integration tests, both for inline patterns and named patterns.
Also, if the pattern has a defined language that differs from the specified language it should probably be an error. --language just changes the default.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/cli/src/commands/apply_pattern.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/commands/apply_pattern.rs
Co-authored-by: Morgante Pell <morgante@grit.io>
Add tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
crates/cli_bin/tests/snapshots/apply__language_option_apply.snapis excluded by:!**/*.snapcrates/cli_bin/tests/snapshots/apply__language_option_conflict_apply.snapis excluded by:!**/*.snapcrates/cli_bin/tests/snapshots/apply__language_option_inline_pattern_apply.snapis excluded by:!**/*.snap
Files selected for processing (2)
- crates/cli/src/commands/apply_pattern.rs (4 hunks)
- crates/cli_bin/tests/apply.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/commands/apply_pattern.rs
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- crates/cli/src/commands/apply_pattern.rs (4 hunks)
- crates/cli_bin/tests/apply.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- crates/cli/src/commands/apply_pattern.rs
- crates/cli_bin/tests/apply.rs
I think it's ok now. |
crates/cli_bin/tests/apply.rs
Outdated
| // from the tempdir as cwd, run marzano apply | ||
| let mut apply_cmd = get_test_cmd()?; | ||
| apply_cmd.current_dir(dir.as_path()); | ||
| // apply_cmd.current_dir(basic_path); |
crates/cli_bin/tests/apply.rs
Outdated
| let mut apply_cmd = get_test_cmd()?; | ||
| apply_cmd.current_dir(dir.as_path()); | ||
| // apply_cmd.current_dir(basic_path); | ||
| apply_cmd.arg("apply").arg("--force").arg("pattern.grit").arg("--language").arg("python"); |
There was a problem hiding this comment.
we only want this feature for inline patterns. If the pattern is defined in a file we should log a warning that the option was unused
crates/cli_bin/tests/apply.rs
Outdated
|
|
||
| #[test] | ||
| fn language_option_conflict_apply() -> Result<()> { | ||
| let pattern = r"language php |
There was a problem hiding this comment.
lets use a currently supported language. I know were adding php soon, but it's not in yet
There was a problem hiding this comment.
My fault, it's ok now.
| @@ -109,6 +109,8 @@ pub struct ApplyPatternArgs { | |||
| /// Clear cache before running apply | |||
| #[clap(long = "refresh-cache", conflicts_with = "cache")] | |||
| pub refresh_cache: bool, | |||
| #[clap(long = "language", alias="lang")] | |||
| pub language: Option<String>, | |||
There was a problem hiding this comment.
Use an enum, so documentation generation and clap will work better.
There was a problem hiding this comment.
Sorry, I can't get your point. Could you detail it?
There was a problem hiding this comment.
Only certain values are valid here, so you should use an enum.
There was a problem hiding this comment.
Ok, I got it. Should we support flavor for this enum?
There was a problem hiding this comment.
You can use this existing enum:
There was a problem hiding this comment.
--language <LANGUAGE>
[possible values: js(js_do_not_use), js(typescript), js, html, css, json, java, csharp, python, markdown(block), markdown(inline), go, rust, ruby, solidity, hcl, yaml, sql, vue, toml, universal]
I think it is.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
crates/cli_bin/tests/snapshots/apply__language_option_file_pattern_apply.snapis excluded by:!**/*.snap
Files selected for processing (2)
- crates/cli/src/commands/apply_pattern.rs (5 hunks)
- crates/cli_bin/tests/apply.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- crates/cli/src/commands/apply_pattern.rs
- crates/cli_bin/tests/apply.rs
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
crates/cli_bin/tests/snapshots/apply__language_option_named_pattern_apply.snapis excluded by:!**/*.snap
Files selected for processing (1)
- crates/cli_bin/tests/apply.rs (1 hunks)
Additional comments: 4
crates/cli_bin/tests/apply.rs (4)
- 2020-2049: The test
language_option_file_pattern_applycorrectly sets up a scenario to test applying a pattern with a specific language option. However, it asserts the command executed successfully without specifically checking if the language option was correctly applied. Consider adding a more detailed assertion to ensure the language option's effect is as expected.- 2051-2081: The test
language_option_inline_pattern_applydemonstrates applying an inline pattern with a language option. Similar to the previous comment, it would be beneficial to include assertions that specifically verify the effect of the language option to ensure it's applied correctly.- 2083-2117: In
language_option_named_pattern_apply, the pattern definition seems to be included directly in the test code. This approach might not accurately reflect how users define and apply named patterns. Consider moving the pattern definition to a separate file or using a predefined pattern to align the test more closely with real-world usage.- 2119-2149: The test
language_option_conflict_applyaims to verify the behavior when there's a conflict between the language option and the pattern's defined language. However, the assertion checks for command failure, which might not be the most informative outcome. It would be more useful to check for a specific error message that clearly indicates the nature of the conflict to the user.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
Cargo.lockis excluded by:!**/*.lockcrates/cli_bin/tests/snapshots/apply__invalid_language_option_apply.snapis excluded by:!**/*.snapcrates/language/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (3)
- crates/cli/src/commands/apply_pattern.rs (5 hunks)
- crates/cli_bin/tests/apply.rs (1 hunks)
- crates/language/src/target_language.rs (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- crates/cli/src/commands/apply_pattern.rs
- crates/cli_bin/tests/apply.rs
Additional comments: 3
crates/language/src/target_language.rs (3)
- 27-29: The addition of
clapandserdedependencies is appropriate for the new functionality.clapis used for parsing command-line arguments, which is essential for the--languageoption in the CLI.serdeis used for serialization, likely for thePatternLanguageenum to facilitate easy conversion to and from string or JSON representations.- 48-48: The
PatternLanguageenum now implementsSerialize, which is crucial for converting enum instances to a format that can be easily stored or transmitted, such as JSON. This change supports the CLI's need to handle language specifications in a flexible and interoperable manner.- 377-429: The implementation of
ValueEnumandPossibleValueforPatternLanguageis a significant enhancement. It allowsPatternLanguageenum variants to be directly used withclapfor command-line argument parsing, improving the usability and maintainability of the code. This change facilitates the new--languageCLI option by enabling users to specify languages in a standardized and type-safe manner.However, it's important to ensure that the representations used in
to_possible_value(e.g.,"js(js_do_not_use)","js(typescript)") are clear and intuitive for users. Consider documenting these representations or providing help text in the CLI to guide users on how to specify languages correctly.Consider adding documentation or help text in the CLI to explain the language representations used in
to_possible_valueto ensure clarity for users.
| @@ -371,6 +374,61 @@ impl PatternLanguage { | |||
| } | |||
| } | |||
|
|
|||
| impl ValueEnum for PatternLanguage { | |||
There was a problem hiding this comment.
I think you could just derive this. See this example: clap-rs/clap#4327
| @@ -285,6 +293,22 @@ pub(crate) async fn run_apply_pattern( | |||
| } | |||
| } | |||
| }; | |||
| if let Some(lang_option) = &arg.language { | |||
| if is_pattern_file(&pattern) { | |||
There was a problem hiding this comment.
I don't think we need to special case pattern files - we should also not allow overriding the language in those cases if there's a conflict.
Similarly I should be able to grit apply a foo.grit that doesn't have a language specified.
Concretely, the purpose of the --language option is to change the default pattern language from JS to whatever is passed.
There was a problem hiding this comment.
@ilevyor Could you agree with this? You comment it before, #69 (comment)
There was a problem hiding this comment.
This is the only remaining change. I'm the tech lead, you don't need @ilevyor to approve.
crates/cli_bin/tests/apply.rs
Outdated
| // Read back the main.py file | ||
| let target_file = dir.join("main.py"); | ||
| let content: String = std::fs::read_to_string(target_file)?; | ||
|
|
||
| // assert that it matches snapshot | ||
| assert_snapshot!(content); |
There was a problem hiding this comment.
You can skip the snapshot here.
| // Read back the main.py file | |
| let target_file = dir.join("main.py"); | |
| let content: String = std::fs::read_to_string(target_file)?; | |
| // assert that it matches snapshot | |
| assert_snapshot!(content); |
There was a problem hiding this comment.
It's to guarante file is unmodified.
There was a problem hiding this comment.
Understood. Instead of a snapshot you should just confirm the current contents match the original contents.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/language/src/target_language.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/language/src/target_language.rs
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- crates/cli/src/commands/apply_pattern.rs (4 hunks)
- crates/cli_bin/tests/apply.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/commands/apply_pattern.rs
Additional comments: 5
crates/cli_bin/tests/apply.rs (5)
- 2021-2048: The test
language_option_file_pattern_applycorrectly tests the scenario where a pattern file is applied with a--languageoption that does not match the file's language, expecting the command to fail. However, it's important to ensure that the pattern file (pattern.grit) explicitly specifies a language or is inherently tied to Python to validate the test's intent.- 2051-2080: The test
language_option_inline_pattern_applyaims to apply an inline pattern with a specified language option. This test assumes the inline pattern is applicable to Python files. To strengthen the test, consider explicitly mentioning in a comment that the pattern is designed for Python syntax to clarify the test's context.- 2083-2116: The test
language_option_named_pattern_applyintroduces a named pattern with a language option. It's crucial to ensure that the named pattern (test_pattern) is defined elsewhere in the test suite or documentation, as it's not inherently clear from the test itself. Providing a reference or ensuring the pattern's existence elsewhere would enhance clarity.- 2119-2150: The test
language_option_conflict_applyeffectively tests the scenario where the--languageoption conflicts with the language specified within the pattern. This test is crucial for ensuring that the CLI tool correctly handles language conflicts. It's well-implemented and serves its purpose effectively.- 2153-2183: The test
invalid_language_option_applychecks the behavior when an invalid language option is provided. This is an important test for validating error handling in the CLI tool. However, ensure that the error message or failure condition is explicitly checked in the test to confirm that the failure is due to the invalid language option and not some other reason.
Close #58
/claim #58
Summary by CodeRabbit
New Features
Tests
Refactor