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

Support gradle kotlin #2680

Merged
merged 3 commits into from
Dec 14, 2020
Merged

Conversation

shakhar
Copy link
Contributor

@shakhar shakhar commented Oct 26, 2020

Gradle Kotlin's build files have a little bit different formatting that Dependabot does not support yet.
In the following PR I done some changes and additions in the existing gradle code to support gradle kotlin files.

These are the main changes:

  • I changed the code to support gradle kotlin by supporting files with .gradle.kts suffix and support all regex parsing that different from groovy.
  • Added some high level and edge cases tests for gradle kotlin

@shakhar shakhar requested a review from a team as a code owner October 26, 2020 12:37
@shakhar shakhar force-pushed the support_gradle_kotlin branch 2 times, most recently from 7b11463 to 83de89d Compare October 26, 2020 13:16
@jurre
Copy link
Member

jurre commented Oct 26, 2020

Thanks for the contribution @shakhar!

I'm a bit worried about the entire test suite being duplicated, that means that we'll have to maintain and run twice the number of test cases, even though the code is only slightly different.

How do you feel about adding a few high level test cases for kotlin, and maybe some specific edge-cases? As long as the code that was introduced is covered that would be sufficient IMO

@shakhar shakhar force-pushed the support_gradle_kotlin branch 5 times, most recently from 4ded747 to 4a0d8f1 Compare October 27, 2020 08:20
@jurre
Copy link
Member

jurre commented Oct 27, 2020

Amazing work on this @shakhar, we'll need a few days to test/review this but it's greatly appreciated!

cc @thepwagner would love to get your 👀 on this as well

Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

Thanks for this! 🎉

I've made some inline comments, my biggest concern is around the syntax for maps not matching what I recall from Kotlin.

gradle/lib/dependabot/gradle/file_fetcher.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_fetcher.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_fetcher.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_parser.rb Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_parser.rb Outdated Show resolved Hide resolved
gradle/spec/fixtures/buildfiles/root_build.gradle.kts Outdated Show resolved Hide resolved
gradle/lib/dependabot/gradle/file_updater.rb Outdated Show resolved Hide resolved
@shakhar
Copy link
Contributor Author

shakhar commented Nov 2, 2020

Hey @jurre and @thepwagner, Is there some news about your testing of this feature?

@thepwagner
Copy link
Contributor

@shakhar sorry for the delay, it may be a few weeks before we can loop back to test this.

@shakhar
Copy link
Contributor Author

shakhar commented Dec 1, 2020

@shakhar sorry for the delay, it may be a few weeks before we can loop back to test this.

@jurre @thepwagner Any updates? (not rushing just checking if you have some news 😃 )

@jurre
Copy link
Member

jurre commented Dec 2, 2020

Hi @shakhar, we're planning to figure out what additional work adding support for kotlin will be on our side, and then hopefully I can give you a better answer. Your work is definitely very much appreciated! 💛

Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this PR @shakhar!

I've been testing it out with main using the bin/dry-run.sh script for some recent Dependabot jobs.

I only found one minor issue which I've highlighted below. I'm running the specs to check reasoning that it's just an RSpec issue.

Once that is fixed, I think we can go ahead and :shipit:

gradle/lib/dependabot/gradle/file_fetcher.rb Show resolved Hide resolved
shakhar and others added 2 commits December 12, 2020 01:33
Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com>
Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

Thanks @shakhar! This looks ready to :shipit: - I'll start rolling forward on a release

@bountin
Copy link
Contributor

bountin commented Dec 23, 2020

Thanks a lot for the work! 🏅
Works nicely for me :)

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.

None yet

5 participants