Pull requests cleanup
Re: Pull requests cleanup
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
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
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.
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.
- AnyOldName3
- Posts: 2674
- Joined: 26 Nov 2015, 03:25
Re: Pull requests cleanup
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
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).
@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
It seems there is a time for another cleanup.
Re: Pull requests cleanup
There is definely a time for cleanup - we have 50 opened PR's on the GitHub only.
- Capostrophic
- Posts: 794
- Joined: 22 Feb 2016, 20:32
Re: Pull requests cleanup
Adding to this, could it be time for another coverity scan?
Re: Pull requests cleanup
I'd prefer to merge shadows first.
- psi29a
- Posts: 5361
- Joined: 29 Sep 2011, 10:13
- Location: Belgium
- Gitlab profile: https://gitlab.com/psi29a/
- Contact:
Re: Pull requests cleanup
Shadows first please...