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
Issue #932 - Does jcabi-github support Android? #1162
Conversation
878819b
to
b611511
Compare
@pecko Many thanks, I will find someone to review it before we merge |
@darkled could you please review this PR |
<groupId>com.jcabi</groupId> | ||
<artifactId>jcabi-manifests</artifactId> | ||
</dependency> | ||
<!-- <dependency> --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pecko do we really need to have this comments here? would it be better to remove these lines completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pecko I don't see any fixing commit from you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it locally.
I will commit when solving all issues.
Fighting with RtDatatypeConverter checkstyle :(
package com.jcabi.github; | ||
|
||
/** | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pecko could you please also remove redundant line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@pecko please see few comments above |
*/ | ||
package com.jcabi.github; | ||
/** | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant empty lines 32 and 37 still exist...
@darkled BTW |
@pecko thank you! If you find issues like this you can report a new defect and someone will try to fix it. |
@rultor merge pls |
@rultor merge pls |
@pecko @yegor256 Oops, I failed. You can see the full log here (spent 3min)
|
@yegor256 try to merge once more please |
@rultor try to merge |
@pecko @yegor256 Oops, I failed. You can see the full log here (spent 10min)
|
@darkled @yegor256 A new error
I will try to fix it. |
@darkled @yegor256 I'm sorry because repeat message, but could you please try to merge this PR once yet? There were a few invalid todos that are fixed. |
@rultor merge once more please |
@rultor try to merge |
@pecko @yegor256 Oops, I failed. You can see the full log here (spent 6min)
|
@darkled @yegor256 Rultor fails in new test:
Will we ignore this test too? |
@darkled yes, seems so |
@yegor256 @darkled How it worked before? This PR introduce nothing related to these failures, errors, todos ... |
@pecko well, it worked pretty fine before. as you can see in the merged PRs. I think the problem came with some of previous merges, since other PRs has the same failures. |
@darkled I want just to understand it better. You claim that integration tests never fail before or they fail rarely? |
@pecko everything what happened before is on github, so you can easily check previous PRs :) I didn't see anything similar to our case. But now there are few PRs with very similar problems. |
@darkled I believe you :) . If they haven't fails before, we have some weird issue that can be in different projects (rultor, jcabi-github, jcabi-http...). I hope, we will fix it soon. |
@pecko This PR is too big and too old, anyway. There is another PR for removing the Manifests mechanisms, so that issue will be solved there. If you care to make another PR with smaller changes (max 5 files, including test changes), that would improve the Android issue, it would be welcome. |
The job is not in WBS, won't close the order |
The ticket: #932
The PR solves two issues that disable using jcabi-github on Android or similar environment:
Android repacks all libraries and the 'JCabi-Version' property and other similar properties doesn't exist in apk.
All RtGithub constructors don't work. They throw:
Solution:
The problem happens when the issue pull requests API #1 is fixed and it is related only to "new RtGithub(user, pwd)" constructor.
Solution:
The class will use the standard DatatypeConverter class if exists or the method(s) copied from DatatypeConverterImpl.
I have tested the following code:
within a Android activity.