search

mashingan / anonimongo

Another Nim pure Mongo DB driver
MIT License
43 stars 5 forks source link

avoid strutils.join because $ has sideEffects on bson structs #24

Closed inv2004 closed 1 year ago

inv2004 commented 1 year ago

fixes https://github.com/mashingan/anonimongo/issues/21

the PR deps on https://github.com/OpenSystemsLab/hmac.nim/pull/11 which is deps on https://github.com/ba0f3/scram.nim/pull/22

mashingan commented 1 year ago

I've sent you invitation as a collaborator. Please rebase after #24 merged and you can merge this. Personally I'd like to support as old as Nim version 1.4, but I understand it's harder to keep the lowest/oldest Nim version. I leave the decision to you and @samsamros.

samsamros commented 1 year ago

Do we have information on people using this package? I'm currently holding code at 1.6.14, but expecting to slowly get to 2.0. Since ORC and threads were announced as possible defaults in 2.0 (which they are now), I've been compiling my code with those flags on on 1.6.x . Anonimongo has worked well with those flags. If we have an idea on what users are doing, may be we can decide whether to bump up the minimum requirement to 1.6.x and keep building from there. If users continue to compile in 1.4.x I would also keep some support for 1.4, and announce as of now that it will lose support in the near future.

inv2004 commented 1 year ago

@samsamros I started to migrate it for my personal needs. But from here https://github.com/inv2004/ttop/ I undertood about versions: many linux distros contains nim 1.6.10 in current repos, that is why I would like to keep 1.6.10 like minimal version, but found that some deps wants 1.6.12.

I would suggest to try again 1.6.10 like minimal if it is possible to make it without a lot of effort

inv2004 commented 1 year ago

looks like tests passed on both 1.6.12, 2.0.0 and 1.6.14 too, but something about docs failed - I do not have idea about it. If @mashingan or @samsamros can help with it

inv2004 commented 1 year ago

@mashingan you can merge if you think it is safe - I see that formally tests are not passed - probably because it is not master branch and it works after merge - IDK. I am afraid to brake master branch

mashingan commented 1 year ago

Go ahead merge this. But if you want to merge to master, preferably please upgrade Anonimongo version in anonimongo.nimble and also in /src/anonimongo/dbops/client.nim and please tag the branch after merging to master. Above treatment only necessary when you decide to release a new tag version, in case you want to accumulate the change before releasing, please merge to branch develop instead. Also when there's actual code changes, if the changes only related to examples, documentation or the likes, it's okay to merge to master without bumping Anonimongo version.

Thank you for the PR.

inv2004 commented 1 year ago

@mashingan Please find that my initial question was about the doc step in the PR which is not passed - it is the reason I did not merge it