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

fix: remove min_level spread throughout the codebase #451

Merged
merged 8 commits into from
Aug 8, 2024

Conversation

morgante
Copy link
Contributor

@morgante morgante commented Aug 8, 2024

Instead of needing to check the visibility level every time we emit a diagnostic, now it is the responsibility of the messenger to determine the level it wants to use for all messages.

This significantly simplifies state and processing and fixes a bug where workflows were showing excessive debug messages even in normal mode.

Summary by CodeRabbit

  • New Features

    • Enhanced visibility management in various functions, allowing for more refined control over message emissions based on visibility levels.
    • Introduced new methods to retrieve minimum visibility levels in several messenger implementations.
  • Refactor

    • Simplified function signatures by removing unnecessary parameters, improving readability and maintainability.
    • Streamlined the internal logic for message handling, reducing complexity in method calls.
  • Bug Fixes

    • Corrected issues with unnecessary parameters in multiple function calls, enhancing the overall clarity and usability of the code.
  • Documentation

    • Improved comments and documentation for new functionality related to visibility levels and message emission.
  • Style

    • Minor adjustments to import formatting for better code organization and readability.

@morgante morgante changed the title Remove emit level Remove min_level spread throughout the codebase Aug 8, 2024
@morgante morgante changed the title Remove min_level spread throughout the codebase fix: remove min_level spread throughout the codebase Aug 8, 2024
@morgante morgante marked this pull request as ready for review August 8, 2024 03:41
@morgante morgante requested a review from a team as a code owner August 8, 2024 03:41
Copy link
Contributor

coderabbitai bot commented Aug 8, 2024

Walkthrough

Walkthrough

The recent changes across multiple files primarily focus on refactoring for improved readability and maintainability. Key modifications include the addition and removal of parameters, particularly concerning visibility levels in messaging functions. These adjustments streamline function signatures and enhance encapsulation, allowing for better control over message handling without altering the overall functionality of the codebase.

Changes

File(s) Change Summary
crates/cli/src/analyze.rs Removed min_level variable from function calls, simplifying signatures while maintaining control flow.
crates/cli/src/commands/apply.rs Added args.apply_pattern_args.visibility to run_apply, enhancing functionality for migration applications.
crates/cli/src/commands/apply_migration.rs Introduced min_level parameter to run_apply_migration, incorporating visibility levels into migration logic.
crates/cli/src/commands/apply_pattern.rs Removed min_level from run_apply_pattern, cleaning up method calls while retaining overall functionality.
crates/cli/src/commands/check.rs Streamlined run_check by removing VisibilityLevels::Supplemental from method calls, enhancing code clarity.
crates/cli/src/commands/parse.rs Added visibility parameter when instantiating JSONLineMessenger, simplifying emission calls by removing visibility from emit.
crates/cli/src/commands/plumbing.rs Included VisibilityLevels::default() parameter, enhancing function capabilities for visibility management.
crates/cli/src/jsonl.rs Added min_level to JSONLineMessenger struct and its constructor, along with get_min_level method for managing message visibility.
crates/cli/src/messenger_variant.rs Enhanced get_min_level method for MessengerVariant, added min_level parameter to create_emitter, simplifying message emission logic.
crates/cli/src/result_formatting.rs Added min_level to FormattedMessager, updated constructors and methods for visibility management.
crates/cli/src/ux.rs Minor formatting adjustments in import statements for better readability.
crates/core/src/test_files.rs Corrected import statement formatting for MatchResult, enhancing clarity.
crates/grit-pattern-matcher/src/pattern/variable.rs Minor formatting change in the anyhow crate import for consistency.
crates/marzano_messenger/src/emit.rs Introduced get_min_level method; removed min_level parameter from emit and apply_rewrite methods, simplifying the interface.
crates/marzano_messenger/src/testing.rs Added get_min_level method for TestingMessenger, streamlining message emission.
crates/wasm-bindings/src/match_pattern.rs Refactored assignment of uses_ai for improved readability without changing logic.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Command
    participant Emitter
    participant Messenger

    User->>Command: Execute Command
    Command->>Emitter: Create with VisibilityLevels
    Emitter->>Messenger: Emit Message
    Messenger->>Emitter: Get Minimum Level
    Emitter->>Messenger: Emit Message with Min Level
    Messenger-->>User: Return Result
Loading

Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c206ec2 and 2add3b8.

Files selected for processing (16)
  • crates/cli/src/analyze.rs (4 hunks)
  • crates/cli/src/commands/apply.rs (1 hunks)
  • crates/cli/src/commands/apply_migration.rs (2 hunks)
  • crates/cli/src/commands/apply_pattern.rs (6 hunks)
  • crates/cli/src/commands/check.rs (5 hunks)
  • crates/cli/src/commands/parse.rs (3 hunks)
  • crates/cli/src/commands/plumbing.rs (2 hunks)
  • crates/cli/src/jsonl.rs (1 hunks)
  • crates/cli/src/messenger_variant.rs (4 hunks)
  • crates/cli/src/result_formatting.rs (5 hunks)
  • crates/cli/src/ux.rs (1 hunks)
  • crates/core/src/test_files.rs (1 hunks)
  • crates/grit-pattern-matcher/src/pattern/variable.rs (1 hunks)
  • crates/marzano_messenger/src/emit.rs (9 hunks)
  • crates/marzano_messenger/src/testing.rs (3 hunks)
  • crates/wasm-bindings/src/match_pattern.rs (1 hunks)
Files skipped from review due to trivial changes (7)
  • crates/cli/src/analyze.rs
  • crates/cli/src/commands/apply_pattern.rs
  • crates/cli/src/commands/check.rs
  • crates/cli/src/ux.rs
  • crates/core/src/test_files.rs
  • crates/grit-pattern-matcher/src/pattern/variable.rs
  • crates/wasm-bindings/src/match_pattern.rs
Additional context used
Learnings (1)
Common learnings
Learnt from: morgante
URL: https://github.com/getgrit/gritql/pull/231

Timestamp: 2024-04-15T05:47:47.743Z
Learning: User morgante prefers not to be asked for documentation.
Additional comments not posted (22)
crates/marzano_messenger/src/testing.rs (2)

37-39: Establish a default visibility level.

The get_min_level method provides a default visibility level of VisibilityLevels::Debug, which is useful for testing scenarios where all messages should be visible.


70-70: Centralize visibility control in emit_from_workflow.

The removal of the explicit visibility level argument in emit_from_workflow suggests that the method now relies on the default behavior of emit, which is a cleaner and more maintainable approach.

crates/cli/src/jsonl.rs (3)

17-17: Add min_level field to JSONLineMessenger.

The addition of the min_level field allows the messenger to manage message visibility levels, enhancing its configurability.


21-29: Update constructor to include min_level.

The constructor now requires a min_level parameter, ensuring that the visibility level is specified upon initialization. This change improves the control over message handling.


35-37: Provide method to retrieve min_level.

The get_min_level method allows retrieval of the current minimum visibility level, which is crucial for controlling message output based on visibility constraints.

crates/cli/src/commands/apply_migration.rs (2)

67-67: Add min_level parameter to run_apply_migration.

The inclusion of the min_level parameter allows the migration function to manage operations based on visibility levels, enhancing its flexibility and control.


79-79: Pass min_level to create_emitter.

Passing the min_level parameter to create_emitter ensures that the emitter respects the specified visibility constraints, aligning with the overall goal of centralizing visibility control.

crates/cli/src/commands/parse.rs (2)

46-48: Centralize visibility management.

The addition of the visibility parameter to JSONLineMessenger centralizes the visibility management, which aligns with the PR objectives of streamlining message handling. This change simplifies the emit calls by removing the need to pass visibility each time.


62-62: Simplified emit method calls.

The removal of the visibility argument from the emitter.emit calls is consistent with the new design where visibility is managed internally by the JSONLineMessenger. This change reduces redundancy and potential errors in setting visibility levels.

Also applies to: 79-79

crates/cli/src/commands/apply.rs (1)

84-84: Enhance visibility control in run_apply.

The addition of args.apply_pattern_args.visibility to the run_apply_migration function call enhances the control over visibility settings during migration applications. This change aligns with the PR's objective to streamline visibility management across the codebase.

crates/cli/src/commands/plumbing.rs (2)

11-11: Import VisibilityLevels for enhanced visibility management.

The import of VisibilityLevels is necessary for the new visibility management logic introduced in the run_plumbing function. This change supports the PR's goal of centralizing visibility control.


319-319: Improve visibility handling in run_plumbing.

The addition of VisibilityLevels::default() as a parameter enhances the function's capability to manage visibility settings, aligning with the PR's objective to centralize and streamline visibility management.

crates/cli/src/messenger_variant.rs (3)

42-54: LGTM!

The get_min_level method is correctly implemented to delegate calls to the respective messenger variant.


Line range hint 55-69: Simplified visibility handling.

The raw_emit method now relies on get_min_level, which simplifies the logic.


Line range hint 251-279: Enhanced configurability with min_level.

The create_emitter function now accepts a min_level parameter, improving emitter configuration.

crates/marzano_messenger/src/emit.rs (3)

30-31: LGTM!

The get_min_level method enhances flexibility by allowing each implementer to define its minimum visibility level.


33-40: Simplified emit method.

The emit method now uses get_min_level to determine visibility, simplifying the interface.


Line range hint 41-50: Encapsulation improved in apply_rewrite.

The method now uses get_min_level, aligning with the new encapsulation approach.

crates/cli/src/result_formatting.rs (4)

305-305: Visibility control added to FormattedMessager.

The min_level field enhances control over message visibility.


Line range hint 315-325: Configurability improved in FormattedMessager::new.

The constructor now accepts a min_level parameter, enhancing configurability.


332-334: Consistent interface in get_min_level.

The method provides a consistent way to retrieve the minimum visibility level.


450-452: Default visibility level in TransformedMessenger.

The get_min_level method provides a consistent interface for visibility levels.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@morgante morgante merged commit c5c8330 into main Aug 8, 2024
12 of 14 checks passed
@github-actions github-actions bot mentioned this pull request Aug 6, 2024
@github-actions github-actions bot mentioned this pull request Sep 11, 2024
@github-actions github-actions bot mentioned this pull request Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant