Page 2 of 3

Re: Pull requests cleanup

Posted: 08 Aug 2018, 08:59
by Zini
Most of the PRs that have piled up are flagged as Discussion, WIP or request for comments. That indicates that the author thinks these PRs aren't ready. Therefore nothing to clean up IMO.

Re: Pull requests cleanup

Posted: 08 Aug 2018, 09:54
by akortunov
Zini wrote: 08 Aug 2018, 08:59 Most of the PRs that have piled up are flagged as Discussion, WIP or request for comments. That indicates that the author thinks these PRs aren't ready. Therefore nothing to clean up IMO.
Just a start:
https://github.com/OpenMW/openmw/pull/844 - Not sure if it still needed.
https://github.com/OpenMW/openmw/pull/1600 - Android port now is a separate fork, so this PR is no more maintained.
https://github.com/OpenMW/openmw/pull/1826 - it would be nice to have your opinion here.
https://github.com/OpenMW/openmw/pull/1847 - the same thing.
https://github.com/OpenMW/openmw/pull/1063
https://github.com/OpenMW/openmw/pull/1720
https://github.com/OpenMW/openmw/pull/1808
https://github.com/OpenMW/openmw/pull/1853
https://github.com/OpenMW/openmw/pull/1852
https://github.com/OpenMW/openmw/pull/1666

Re: Pull requests cleanup

Posted: 09 Aug 2018, 12:42
by Zini
1852 and 1853 are already scheduled for my the next regular merge on GitHub (today). I will do a quick review for 1720 now that the WIP tag is gone.

1063 looks complex. Not sure about it. But since several devs seem to be in favour of merging it we can as well merge it and see if anything breaks. I'll add it to my queue.

1847 still has the macro issue. And maybe some others. Will get back to you on the PR itself.

1826, 1808 and 1666 are all related to animation/the NIF format. I have absolutely no clue about this stuff and definitely not the time to familiarise myself enough with it to contribute anything to the discussion (really missing scrawl in moments like this). Since the WIP/discussion tags on two of these were removed and devs had overall enough time to review them, I will add them to my merge queue.

I don't have an android device and therefore I have no way to test 1600. But yeah, probably not needed now. Should we close it?

844 sounds more like something for psi29a.

Re: Pull requests cleanup

Posted: 09 Aug 2018, 12:52
by AnyOldName3
I think 844 is being kept around so that when someone else tries to do a better version in the future, they'll have access to a lot of discussion about it.

Re: Pull requests cleanup

Posted: 09 Aug 2018, 12:58
by Zini
Seems a bunch of PRs are missing change log entries.

@AnyOldName3 Kinda make sense. But do we need to keep the PR open for that? It doesn't go away just because it is closed. Linking it from somewhere should do the job even if it is closed (I have no idea from where though).

Re: Pull requests cleanup

Posted: 25 Dec 2018, 17:08
by akortunov
It seems there is a time for another cleanup.

Re: Pull requests cleanup

Posted: 17 Feb 2019, 09:40
by akortunov
There is definely a time for cleanup - we have 50 opened PR's on the GitHub only.

Re: Pull requests cleanup

Posted: 17 Feb 2019, 10:34
by Capostrophic
Adding to this, could it be time for another coverity scan?

Re: Pull requests cleanup

Posted: 17 Feb 2019, 11:36
by akortunov
Capostrophic wrote: 17 Feb 2019, 10:34 Adding to this, could it be time for another coverity scan?
I'd prefer to merge shadows first.

Re: Pull requests cleanup

Posted: 17 Feb 2019, 20:35
by psi29a
Shadows first please...