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

Keep gem in fit #65

Closed

Conversation

numbata
Copy link
Contributor

@numbata numbata commented Jan 31, 2024

  • Bump ruby version for the danger workflow step up to 3.0.
  • Fix rubocop offense

@numbata numbata force-pushed the bump_rubocop_workflow_version branch from fc0cba4 to 53af52f Compare January 31, 2024 14:56
@numbata
Copy link
Contributor Author

numbata commented Jan 31, 2024

It looks like the ruby-grape-danger should bump it's own dependency on the danger gem. Because of that:

        rest_inline_violations = submit_inline_comments!({
          danger_id: danger_id,
          previous_violations: previous_violations
        }.merge(**inline_violations))

The "new" Ruby version sees this explicitly defined hash as a position argument, not as part of keyword arguments. But the definition of the submit_inline_comments method expects only keyword arguments and no any positional.

 def submit_inline_comments!(warnings: [], errors: [], messages: [], markdowns: [], previous_violations: [], danger_id: "danger")

Of course I can also create a PR for the ruby-grape-danger gem, but is it worth it? Seems like other gems in the grape-* family just get rid of this workflow step.

@mscrivo wdyt?

@mscrivo
Copy link
Collaborator

mscrivo commented Jan 31, 2024

It looks like the ruby-grape-danger should bump it's own dependency on the danger gem. Because of that:

        rest_inline_violations = submit_inline_comments!({
          danger_id: danger_id,
          previous_violations: previous_violations
        }.merge(**inline_violations))

The "new" Ruby version sees this explicitly defined hash as a position argument, not as part of keyword arguments. But the definition of the submit_inline_comments method expects only keyword arguments and no any positional.

 def submit_inline_comments!(warnings: [], errors: [], messages: [], markdowns: [], previous_violations: [], danger_id: "danger")

Of course I can also create a PR for the ruby-grape-danger gem, but is it worth it? Seems like other gems in the grape-* family just get rid of this workflow step.

@mscrivo wdyt?

yeah I remember running into this before, which is why I set the ruby version back to 2.7 for danger. I think it's best to just to do that. Seems grape still runs danger and it uses 2.7 as well: https://github.com/ruby-grape/grape/actions/runs/7609193746/job/20719891863?pr=2414

@numbata
Copy link
Contributor Author

numbata commented Jan 31, 2024

The problem here is that the grape-swagger gem requires ruby version 3.x. And that's why the grape-swagger-entity is forced by danger to use that version as well. Even though it is not so necessary.

While the grape gem itself has no such dependencies and free to choose which Ruby version to use.

@mscrivo
Copy link
Collaborator

mscrivo commented Feb 1, 2024

@dblock any plans to make danger Ruby 3.x compatible? Any suggestions on what we should do here?

@numbata
Copy link
Contributor Author

numbata commented Feb 1, 2024

The original'danger' is already compatible. It's just grape's danger locked on outdated version of it.

@mscrivo
Copy link
Collaborator

mscrivo commented Feb 2, 2024

Fixed in #66

@mscrivo mscrivo closed this Feb 2, 2024
@numbata
Copy link
Contributor Author

numbata commented Feb 2, 2024

Thank for such active support! 👍

fyi: my attempt (in early version state) to achieve ruby 3.x support, but keep the ruby 2.x support.
ruby-grape/danger@master...numbata:ruby-grape-danger:bump_dependencies

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.

2 participants