knowm / XChange

XChange is a Java library providing a streamlined API for interacting with 60+ Bitcoin and Altcoin exchanges providing a consistent interface for trading and accessing market data.
http://knowm.org/open-source/xchange/
MIT License
3.85k stars 1.94k forks source link

large backlog of pull requests #4783

Closed douggie closed 10 months ago

douggie commented 11 months ago

Hi,

we seem to have a large backlog of PR's?

Any thoughts on how we can get through them and get them merged?

Happy to help out any way I can

bigscoop commented 11 months ago

I was thinking of creating the fork where the PR's can be approved (or at least reviewed). Plenty of PR's are still pending from me as well.

Would someone support the idea?

douggie commented 11 months ago

What is your thinking around needing a fork.

I think the first thing, is to somehow poriortise them, I guess people will start reviewing stuff that is important to them, for me would be the bybit v5 for example, then once it is reviewed by some contributors, then the core team can be tagged to look at it.

but if no review by other contributors, then no merging, as I guess it is not that important.

Douggie

On Thu, Nov 2, 2023 at 11:28 AM Dmitri Karpovich @.***> wrote:

I was thinking of creating the fork where the PR's can be approved (or at least reviewed). Plenty of PR's are still pending from me as well.

Would someone support the idea?

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1790550556, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBUO2REXMBCJHL7FZSG4PTYCN7U7AVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJQGU2TANJVGY . You are receiving this because you authored the thread.Message ID: @.***>

bigscoop commented 11 months ago

Unfortunately there is no activity from core team since couple of months. That makes me think of creating the fork

douggie commented 11 months ago

ahhhh!!!!!! i see.

On Thu, Nov 2, 2023 at 11:42 AM Dmitri Karpovich @.***> wrote:

Unfortunately there is no activity from core team since couple of months. That makes me think of creating the fork

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1790568595, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBUO2RQT6XFEEFB74XUZVLYCOBJLAVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJQGU3DQNJZGU . You are receiving this because you authored the thread.Message ID: @.***>

douggie commented 11 months ago

So options

Option 1 - Create a fork with new approvers, this seems a bit nuts, just add more approvers to existing project. Option 2 - Ensure that a PR has to be reviewed by a contributer before it is reviwed by an approver, this kinda ensures the prioity is self selecting. Option 3 - Add more approvers to the existing project.

I kinda thinking a combination of 2 and 3. @timmolter @makarid @npomfret @mmazi what do you guys think?

rizer1980 commented 11 months ago

Hello.

I agree that PR is viewed poorly. Also, important!, somebody need to clear up a huge number of old error reports. Option 1 - IMHO, bad. Project stratification, unclear prospects. I vote for 2 and 3

douggie commented 11 months ago

ping @walec51 @timmolter @makarid thougths?

makarid commented 11 months ago

Hello everyone. I guess the best solution is everyone to fork his/her own version of the project and if one is willingly to fork the whole project and maintained it for the long term we can just use his/her fork. @Tim Molter @.***> had said in the past that he had no time to give to the project, totally understood, I will respect that.

On Tue, Nov 7, 2023, 2:03 PM douggie @.***> wrote:

ping @walec51 https://github.com/walec51 @timmolter https://github.com/timmolter @makarid https://github.com/makarid thougths?

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1798370482, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHIWQ7YAT36LFXQXRHOTUBTYDIPSVAVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYGM3TANBYGI . You are receiving this because you were mentioned.Message ID: @.***>

bigscoop commented 11 months ago

@makarid another option would be to extend the list of maintainers. what do you think about it?

douggie commented 11 months ago

Given everything is set up ci etc , can we not just add maintainers, I am happy to maintain it and take over from tim.

On Tue, Nov 7, 2023, 12:11 PM Makarid @.***> wrote:

Hello everyone. I guess the best solution is everyone to fork his/her own version of the project and if one is willingly to fork the whole project and maintained it for the long term we can just use his/her fork. @Tim Molter @.***> had said in the past that he had no time to give to the project, totally understood, I will respect that.

On Tue, Nov 7, 2023, 2:03 PM douggie @.***> wrote:

ping @walec51 https://github.com/walec51 @timmolter https://github.com/timmolter @makarid https://github.com/makarid thougths?

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1798370482, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AHIWQ7YAT36LFXQXRHOTUBTYDIPSVAVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYGM3TANBYGI>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1798382102, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBUO2UUVVMIG5CIEKV6KTLYDIQQ5AVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYGM4DEMJQGI . You are receiving this because you authored the thread.Message ID: @.***>

rizer1980 commented 11 months ago

fork is the last thing. We need a person who will carry the project so that the majority of users start using its version. Otherwise, there will be a bunch of forks, which over time will lose any ability to merge them with the main branch. This will definitely not benefit the project.

I really don't understand why not give a few more people the opportunity to service the project? yes, the quality may suffer a little, but the project will remain a single whole and will continue to develop...

douggie commented 11 months ago

I am happy to take on Tim's role

On Tue, Nov 7, 2023, 2:06 PM Ilya Smirnov @.***> wrote:

fork is the last thing. We need a person who will carry the project so that the majority of users start using its version. Otherwise, there will be a bunch of forks, which over time will lose any ability to merge them with the main branch. This will definitely not benefit the project.

I really don't understand why not give a few more people the opportunity to service the project? yes, the quality may suffer a little, but the project will remain a single whole and will continue to develop...

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1798591520, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBUO2UH6NVYBZKY3WI4PQTYDI55FAVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYGU4TCNJSGA . You are receiving this because you authored the thread.Message ID: @.***>

douggie commented 11 months ago

@makarid are you able to add mainters? May be add me, @bigscoop @rizer1980 as a start? Any others we think?

My start would be 1) People have to get their PR's reviewed by another contributor 2) the maintainers do the final reivew and merge.

makarid commented 11 months ago

I totally agree to add more maintainers but I don't have access to do it. @Tim Molter @.***> and I think @badgerwithagun can

On Wed, Nov 8, 2023, 9:17 AM douggie @.***> wrote:

@makarid https://github.com/makarid are you able to add mainters? May be add me, @ bigscoop @rizer1980 https://github.com/rizer1980 as a start?

My start would be

  1. People have to get their PR's reviewed by another contributor
  2. the maintainers do the final reivew and merge.

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1801222578, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHIWQ7Z6C4QDMNCSRHSVAA3YDMWZTAVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBRGIZDENJXHA . You are receiving this because you were mentioned.Message ID: @.***>

timmolter commented 11 months ago

I'm happy to add more maintainers. Who should I add?

douggie commented 11 months ago

@timmolter pleased you are alive! Great news on adding more matainers, forking the repo seems a bit of e nuclear option.

May be add @bigscoop @rizer1980 and @douggie as a maintainers for a start? This is just based on who has a few PR's out there. @bigscoop @rizer1980 or anyone else any thoughts on who should be added?

@timmolter I am happy to take over the whole maintainer thing (i.e. your role), i.e. ci pipelients etc etc, if you want to hand over the reins fully, we can do a chat some time and share any passwords to switch over users etc etc if I need more access than just maintainer such as adding other maintainers

Douggie

douggie commented 11 months ago

@bigscoop @rizer1980 if you have any pressing PR's can you get them reviwered by another contributer first.

I will review the following PR's as I would like them in https://github.com/knowm/XChange/pull/4771 https://github.com/knowm/XChange/pull/4766 https://github.com/knowm/XChange/pull/4733

If anyone is able to review some of mine that would be great

https://github.com/knowm/XChange/pull/4785 https://github.com/knowm/XChange/pull/4780

bigscoop commented 10 months ago

@douggie I think I still can't neither review nor merge anything because of missing role

walec51 commented 10 months ago

sorry for my absence - I'm not working in the crypto space now and didn't have time to review

we should choose new maintainers based on their experience and availability

looking at https://github.com/knowm/XChange/graphs/contributors

I'm in favor of adding @douggie @makarid @badgerwithagun

as for

@rizer1980 - I'm sorry but I do not see you on the contributors page and you should build more experience before becoming a maintainer

@bigscoop - I see you have your first contributions in the project but I think it would be a little early for a maintainer role, but a couple of more PRs and experience and I think you should be granted that right

rizer1980 commented 10 months ago

Hello, @walec51 Hurray!, I understand that we have someone to approve the PR, thx. I don't understand why I'm not on this page...

I have 16 PRs (11 approved), plus about 10 more ready, but not published for various reasons. Yes, I don’t have much experience in Xchange, but I wasn’t going to go somewhere I don’t know. Clean up the backlog of issue, approve the code that I fully understand.

timmolter commented 10 months ago

@rizer1980 it's because I need to manually update that page during the release process and I may have forgotten recently. You'll be on there on the next release.

timmolter commented 10 months ago

I've added @douggie and @makarid . @badgerwithagun was already able to merge PRs.

@earce @jheusser @badgerwithagun @walec51 Shall I remove you as being able to merge PRs (write role)?

timmolter commented 10 months ago

Since it's been a long time since XChange had a release, we should try to get one out soon. I will publish it once I get the green light, and I'll also update the contributor's list at that time.

douggie commented 10 months ago

Thanks Tim! I am back next week and will get on zee merges.

If any contributor and review other people's prs first will really help.

I will look to do a release.

On Mon, Nov 13, 2023, 4:01 PM Tim Molter @.***> wrote:

Since it's been a long time since XChange had a release, we should try to get one out soon. I will publish it once I get the green light, and I'll also update the contributor's list at that time.

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1808452508, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBUO2Q2EOWB23AZQEHHO63YEI74PAVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYGQ2TENJQHA . You are receiving this because you were mentioned.Message ID: @.***>

makarid commented 10 months ago

Thank you very much @timmolter !

walec51 commented 10 months ago

I plan to return to an active role next year so it would be nice if I could keep my status.

ps. Lets all remember that we should not merge our own PR - some other maintainer must approve it.

Please also try to maintain best practices described on our wiki: https://github.com/knowm/XChange/wiki

Keep backward comparability and do not let non generic features slip to generic interfaces and DTOs.

rizer1980 commented 10 months ago

As about the release. In my opinion, everything is fine with us, there were reports of problems with [Binance Us] - but now everything seems to be stable.

timmolter commented 10 months ago

Glad to see renewed activity here! Let's aim for a release late next week. Need to get lots of open PRs approved and merged!

douggie commented 10 months ago

Back Monday so will start on a few, gate, bybit v5 will be the first ones I will review

On Tue, Nov 14, 2023, 10:38 AM Tim Molter @.***> wrote:

Glad to see renewed activity here! Let's aim for a release late next week. Need to get lots of open PRs approved and merged!

— Reply to this email directly, view it on GitHub https://github.com/knowm/XChange/issues/4783#issuecomment-1809952821, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBUO2VTFHTLIT7U245WXUTYENCZLAVCNFSM6AAAAAA6WBLGJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBZHE2TEOBSGE . You are receiving this because you were mentioned.Message ID: @.***>

douggie commented 10 months ago

@timmolter not sure I have maintainer access yet, was just going to review and merge some PR's but no luck

![Uploading Screenshot 2023-11-16 at 21.06.43.png…]()

timmolter commented 10 months ago

Hi @douggie The invite was sent but not accepted. I resent the invite.

Screenshot 2023-11-28 at 10 55 33 AM
douggie commented 10 months ago

thanks @timmolter! I have the merge power now!

Screenshot 2023-11-28 at 09 57 20

timmolter commented 10 months ago

Great! I hope to cut a new release this week.

timmolter commented 10 months ago

5.1.1 released.