Removing deprecated warnings messages
AbandonedPublic

Authored by lbiaggi on Jan 22 2020, 11:15 PM.

Details

Reviewers
kossebau

Diff Detail

Repository
R32 KDevelop
Branch
removing-warnings (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21538
Build 21556: arc lint + arc unit
lbiaggi created this revision.Jan 22 2020, 11:15 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptJan 22 2020, 11:15 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
lbiaggi requested review of this revision.Jan 22 2020, 11:15 PM
kossebau requested changes to this revision.Jan 22 2020, 11:40 PM
kossebau added a subscriber: kossebau.

Hi @lbiaggi, welcome to contributing to KDevelop, thanks for your first(?) patch proposed here. I agree with your goal to make the code more modern :)

Sadly I have to start with some nitpicks about this patch, but you are here to also learn what others think about development, right? :)

First thing: the commit message is mainly something read by people iin the future. When will they read it, and how much help will they have from the text we give to them? The current text describes the current initial motivation "get rid of warnings". That though is not our actual motivation. We rather want to make the code use the recommended & latest API, when possible. That should be noted here.

But then there is also a mix of changes happening in this very patch which are done for different reasons.
E,g the formatting changes or the reordering of includes. How are they relating to the actual commit message? I guess there is no relation. So they will have to be removed from this very patch.

Then, ideally each porting to new API is done in separate commits, one per API change ported to. This makes commits more easy to read when looking back from the future, and the commit message can be precise and short about the very change. Do a line-by-line git blame call and see yourself how useful precise & compact commits and messages are, and how confusing commits with rather unrelated mass changes are and respective unhelpfll commit messages.
So please do separate commits for each case.

Also did you not mention how you created this patch, and if you tested your changes, and how?

This revision now requires changes to proceed.Jan 22 2020, 11:40 PM

https://invent.kde.org/kde/kdevelop/merge_requests/93 is the replacement of this patch, right?
If so, please also close this review here as discarded :)

lbiaggi abandoned this revision.Jan 24 2020, 5:07 PM