Code style #2447
Code style #2447
Conversation
|
|
The earlier it will be merged, the less problems we will have |
I agree with you but first we need to merge some 2.10 PRs that are pending from almost one month ago and waiting for code review. Can you check #2429, in which you requested some changes and I applied them? |
.idea/codeStyles/Project.xml
Outdated
| <component name="ProjectCodeStyleConfiguration"> | ||
| <code_scheme name="Project" version="173"> | ||
| <option name="LINE_SEPARATOR" value=" " /> | ||
| <option name="RIGHT_MARGIN" value="150" /> |
There was a problem hiding this comment.
150 is too much IMHO as well as Google Java Style Guide talks about 100 as column limit and we had 120 before so I would keep it in 120. I know we already changed this in the library but I will probably set it again to 120.
There was a problem hiding this comment.
you will generate more line breaks, even though you see space right to line
There was a problem hiding this comment.
Generating more line breaks is not something bad most of the time, it makes the code more readable.
There was a problem hiding this comment.
120 is fine when reading changes on GH PRs as well. Can you set it to 120 for now?
There was a problem hiding this comment.
Ok, I will do. But I wait, till a final merge can be done within minutes or hours, because I will start a final reformat after this. And to be honest, I've no motivation to do it again and again ...
Please have a look on how much conflicts I got after 12 days !
There was a problem hiding this comment.
120 is fine when reading changes on GH PRs as well. Can you set it to 120 for now?
Here i apply it
|
https://issuetracker.google.com/issues/78097962 Changed This fix should be available in Android Studio 3.5 Canary 4 and all subsequent releases. |
|
This will be the next PR to merge so you can start to solve the conflicts |
Please merge it immediate |
|
Code approved, I have cloned the code, installed the apk and tried some basic operations just in case. @jesmrec it's up to you to perform extra checks, there's no new logic though |
|
Cleanup made in previous PR has made conflict against the current one. Needed to fix to pass a tiny QA stage. @hannesa2 please take a look and fix it ( |
|
Hey guys, I'm disappointed, probable I've an other understanding from "next".
You should merge it immediate. |
|
Your understanding of next is correct, no worries. Here, it was my fault so i ask my mates to review the other PR before this one, which is really the most important for the incoming release. And i'm sorry, but we are not available 24/7 (and i guess you are not either, we do never ask you to rebase "immediatly"). You are already a member of the repository, so this continuous rebasing we are asking you are in the last days, and this will be a task to be done for anyone in the team... please patience. Your contributions are very welcome but, as you already know, we follow some practices and it is not in our plans to skip them. Please, rebase the branch and i will be aware to check it and then (if there is no problem from QA), merging. Edit: it is already rebased, so i will go for it |
|
Approved... moving forward. |
|
First of all: Thank you for merging ! Anyway now we have all a pain with rebase, I did it with all my 7 remaining PRs and when you run into merge conflicts (you will do it for sure !) you should do a To be honest, I'm happy too, when most of my PRs are merged soon, to hopefully concentrate to more important tasks, instead of error searching PRs |
|
@hannesa2 Thank you very much for your valuable contributions, your honest feedback and your patience! The projects evolves thanks to your effort, not only by the lines of code you add. 🌷 |
similar to owncloud/android-library#224