Pull requests cleanup

Everything about development and the OpenMW source code.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Pull requests cleanup

Post 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.
User avatar
akortunov
Posts: 899
Joined: 13 Mar 2017, 13:49
Location: Samara, Russian Federation

Re: Pull requests cleanup

Post 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
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Pull requests cleanup

Post 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.
User avatar
AnyOldName3
Posts: 2666
Joined: 26 Nov 2015, 03:25

Re: Pull requests cleanup

Post 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.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Pull requests cleanup

Post 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).
User avatar
akortunov
Posts: 899
Joined: 13 Mar 2017, 13:49
Location: Samara, Russian Federation

Re: Pull requests cleanup

Post by akortunov »

It seems there is a time for another cleanup.
User avatar
akortunov
Posts: 899
Joined: 13 Mar 2017, 13:49
Location: Samara, Russian Federation

Re: Pull requests cleanup

Post by akortunov »

There is definely a time for cleanup - we have 50 opened PR's on the GitHub only.
User avatar
Capostrophic
Posts: 794
Joined: 22 Feb 2016, 20:32

Re: Pull requests cleanup

Post by Capostrophic »

Adding to this, could it be time for another coverity scan?
User avatar
akortunov
Posts: 899
Joined: 13 Mar 2017, 13:49
Location: Samara, Russian Federation

Re: Pull requests cleanup

Post 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.
User avatar
psi29a
Posts: 5355
Joined: 29 Sep 2011, 10:13
Location: Belgium
Gitlab profile: https://gitlab.com/psi29a/
Contact:

Re: Pull requests cleanup

Post by psi29a »

Shadows first please...
Post Reply