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

Add more detailed output with --verbose flag #25

Merged
merged 6 commits into from
Oct 30, 2021
Merged

Conversation

nshki
Copy link
Owner

@nshki nshki commented Oct 11, 2021

Discussion

#24.

Overview

This PR adds a --verbose flag to Chusaku and outputs changes in detail when used with that flag.

Example output:

[test/mock/app/controllers/api/burritos_controller.rb:4]

Before:
```ruby
  # @route POST /api/burritos (burritos)
```

After:
```ruby

```

[test/mock/app/controllers/api/burritos_controller.rb:5]

Before:
```ruby
  def create; end
```

After:
```ruby
  # @route POST /api/burritos (burritos)
  def create; end
```

[test/mock/app/controllers/api/tacos_controller.rb:4]

Before:
```ruby
  def show; end
```

After:
```ruby
  # @route GET / (root)
  # @route GET /api/tacos/:id (taco)
  def show; end
```

[test/mock/app/controllers/api/tacos_controller.rb:6]

Before:
```ruby
  # This is an example of generated annotations that come with Rails 6
  # scaffolds. These should be replaced by Chusaku annotations.
  # POST /api/tacos
  # PATCH/PUT /api/tacos/1
```

After:
```ruby
  # This is an example of generated annotations that come with Rails 6
  # scaffolds. These should be replaced by Chusaku annotations.
```

[test/mock/app/controllers/api/tacos_controller.rb:10]

Before:
```ruby
  def create; end
```

After:
```ruby
  # @route POST /api/tacos (tacos)
  def create; end
```

[test/mock/app/controllers/api/tacos_controller.rb:12]

Before:
```ruby
  # Update all the tacos!
  # @route this should be deleted, it's not a valid route.
  # We should not see a duplicate @route in this comment block.
  # @route PUT /api/tacos/:id (taco)
  # But this should remain (@route), because it's just words.
```

After:
```ruby
  # Update all the tacos!
  # We should not see a duplicate @route in this comment block.
  # But this should remain (@route), because it's just words.
```

[test/mock/app/controllers/api/tacos_controller.rb:17]

Before:
```ruby
  def update; end
```

After:
```ruby
  # @route PUT /api/tacos/:id (taco)
  # @route PATCH /api/tacos/:id (taco)
  def update; end
```

[test/mock/app/controllers/api/tacos_controller.rb:19]

Before:
```ruby
  # This route doesn't exist, so it should be deleted.
  # @route DELETE /api/tacos/:id
```

After:
```ruby
  # This route doesn't exist, so it should be deleted.
```

[test/mock/app/controllers/waterlilies_controller.rb:4]

Before:
```ruby
  def show; end
```

After:
```ruby
  # @route GET /waterlilies/:id (waterlilies)
  # @route GET /waterlilies/:id (waterlilies2)
  # @route GET /waterlilies/:id {blue: true} (waterlilies_blue)
  def show; end
```

[test/mock/app/controllers/waterlilies_controller.rb:6]

Before:
```ruby
  def one_off; end
```

After:
```ruby
  # @route GET /one-off
  def one_off; end
```

Chusaku has finished running.

@nshki nshki self-assigned this Oct 11, 2021
@nshki
Copy link
Owner Author

nshki commented Oct 11, 2021

@G-Rath here's a draft PR for you to check out. It doesn't include the GitHub Action setup quite yet, but wanted to see how you like the new --verbose output.

@G-Rath
Copy link
Contributor

G-Rath commented Oct 16, 2021

Looking good! I'm not sure how suitable it is for Github Actions, because you can't add to messages (so at best it'd just show the whole block without explaining what it's actually doing) but that might be fine and the best we can get anyway 🤔

@nshki
Copy link
Owner Author

nshki commented Oct 16, 2021

@G-Rath I was reading over the documentation for Problem Matchers and originally thought we’d be able to just write some regex that captures all the changes to annotate them, but it seems like the limitations (annotation count limits) actually hinder this quite a bit especially for bigger codebases. 😢

As an alternative though, you can definitely set Chusaku to run in a GitHub Actions workflow and fail when there are pending annotations using the —exit-on-error-on-annotation and —dry-run flags. If you want the changes outputted, this new —verbose flag will give a nice overview in the workflow output.

@G-Rath
Copy link
Contributor

G-Rath commented Oct 16, 2021

@nshki I don't think those limitations actually factor into this all that much, since they're not per PR - so e.g. fixing some of what caused the current annotations (for any tool) will result in more showing up when you push up the fixes for those.

(golangci-lint is a good example of this flow - by default it only shows ~20 errors total rather than give you a possible massive list of them; so as you action those, you see new ones on the next run)

Additionally, long term I'd expect most changes to be only affecting a small number of routes (tbh I'd only expect 1-3) for existing projects that are having features and bug fixes applied; so this would mean you'd only have a small number of annotations that actually need changing.

Finally, those limits are GHA specific and could be changed in future - overall, I think tools/clis can focus on outputting information in a way that can be parsed by machines without worrying about specific UIs.

@nshki
Copy link
Owner Author

nshki commented Oct 16, 2021

@G-Rath Great points! I was only considering the extreme case which would only really happen when Chusaku is introduced to a large codebase without actually running it in the same PR.

I think this verbose output is sufficient enough to get parsed for a GHA setup. I'll spend a little time testing Problem Matchers with a separate Rails project and will tweak the output if needed.

@nshki
Copy link
Owner Author

nshki commented Oct 18, 2021

I've been testing the following GHA workflow in a private Rails repo:

.github/chusaku-annotations-matcher.json:

{
  "problemMatcher": [
    {
      "owner": "chusaku",
      "pattern": [
        {
          "regexp": "^\\[(.+)\\/(.+):(\\d+)\\]\\n[\\S|\\s]+?\\n---NEW\\n([\\S|\\s]*?(?=(\\n\\[.+:\\d+\\])|\\n\\n))",
          "fromPath": 1,
          "file": 2,
          "line": 3,
          "message": 4
        }
      ]
    }
  ]
}

.github/workflows/chusaku.yml:

name: Chusaku
on: [pull_request]
jobs:
  chusaku:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - name: Set up Ruby
        uses: ruby/setup-ruby@v1
        with:
          ruby-version: 2.7.4
          bundler-cache: true
      - name: Run Chusaku
        run: |
          echo "::add-matcher::.github/chusaku-annotations-matcher.json"
          bundle exec chusaku --dry-run --verbose

I haven't been able to get proper annotations yet, so still debugging, but I've tested the regex via Rubular and have gotten the correct match groups there.

@nshki
Copy link
Owner Author

nshki commented Oct 30, 2021

While I haven't cracked the GitHub Actions annotations portion, I'm going to go ahead and merge this PR since I think this will give people more parse-able output for any automation they'd like to run in conjunction with Chusaku.

@nshki nshki marked this pull request as ready for review October 30, 2021 22:40
@nshki nshki merged commit 58e93d6 into main Oct 30, 2021
@nshki nshki deleted the add-more-detailed-output branch October 30, 2021 22:41
# 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.

2 participants