-
-
Notifications
You must be signed in to change notification settings - Fork 211
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 Most Gradle Deprecation Warnings & Update Wrapper #331
Conversation
I added an update to Gradle |
Codecov Report
@@ Coverage Diff @@
## master #331 +/- ##
=======================================
Coverage 88.25% 88.25%
=======================================
Files 712 712
Lines 5945 5945
Branches 240 240
=======================================
Hits 5247 5247
Misses 649 649
Partials 49 49
Continue to review full report at Codecov.
|
This PR breaks import in IntelliJ. It should probably not be merged at the moment. This is the stack trace shown in IntellJ, I assume we are hitting a bug: IntelliJ Gradle Import Stacktrace
|
7e6401c
to
61cc5ce
Compare
The IntelliJ error seems to be this bug: https://youtrack.jetbrains.com/issue/IDEA-228368 |
Also blocked by robstoll/kbox#26 |
dropping Please also change CONTRIBUTING; it mentions |
@jGleitz Looks good, the only change missing is |
You could move the changes of the nodejs plugin and the change to npx to a separate PR |
@jGleitz please rebase on master, it depends on new versions of kbox and niok |
fa5f461
to
b37ee39
Compare
Would you prefer that? I was too lazy to disentangle the changes from my other ones. But I will, of course, do it if you prefer. |
Nah, fine just though maybe you want that things go into the master faster :) |
@robstoll: There is a problem with
|
I guess this is still the case correct? If so then I suggest we release Atrium 0.9.1 without this merged (ah right, that was the reason why I though you could move the npx related stuff into an own PR, but still up to you) |
@robstoll: The dependencies should be fine now. However, the build will still fail because the sample projects depend on atrium 9 (which has the bad versions). I propose that we publish atrium to a local repository in the The advantage is that the samples then also function as some kind of integration test. |
Ah shoot, I totally forgot about the IntelliJ bug. Yes, it still happens. Nothing depends on this PR, so I would just go ahead and update the versions and then release atrium 0.9.1 without this PR. |
I guess you talk about jasmine and mocha right? We should no longer run into this problem as soon as they are really independent projects as the other samples (#245) |
No, all samples.
For this specific PR, we could circumvent the issue by letting the samples still use Gradle, 5, that’s right. But I still think it would be valuable to have the samples use the current state of the codebase. This way, we would also catch in CI when the samples are not up-to-date. |
3653e81
to
425b9eb
Compare
I see your point, let's move the discussion to atrium-roadmap. I would prefer if we can tell dependabot to update also the dependencies of the samples including the wrapper (https://github.com/dependabot/feedback/issues/237) |
@jGleitz Jetbrains claims that all import issues where fixed for samples users provided (I mentioned this PR) in the meantime. I don't have time and in the end we need to wait until the fix lands in a stable version, but if you have time and want you could checkout the latest EAP and see if it is true |
Import does indeed succeed with IntelliJ 2020.1 EAP. So I think we will wait with this PR until JetBrains releases it. They should also adopt a continuous release strategy… :/ |
I think we should add to CONTRIBUTING.md that IntelliJ 2020.1 is required and link to the bug |
# Conflicts: # .github/workflows/java-windows.yml # build.gradle
IntelliJ 2020.1 is out and I have updated this Pull Request. I think it is finally ready to merge. Happy Easter! 🐰 |
Sweet, thanks for your contribution. |
This PR fixes Gradle deprecation warnings and should make atrium almost ready for Gradle 7.
npx
tasks instead of installing node dependencies explicitlyI could not fix one warning, which seems to be caused by an
expectedBy
dependency. I guess this is out of our control.Stacktrace of the remaining error
closes #217
I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.