search

pharo-nosql / mongotalk

A Pharo driver for MongoDB
MIT License
19 stars 13 forks source link

Replace OSProcess by OSSubProcess #96

Closed tinchodias closed 3 years ago

tinchodias commented 3 years ago

Try replacing OSProcess by OSSubProcess, which in version v1.3.0 is loadable in all current Pharo versions.

Intends to fix #94.

tinchodias commented 3 years ago

Fine, let me try.... Replica set tests use it and I consider that important functionality to be covered by tests. I can try creating a special group to have them separated from other tests...

tinchodias commented 3 years ago

Effectively, there could be a new Mongo-Tests-ReplicaSet package like this:

Screen Shot 2021-01-19 at 11 22 18

But still, Mongo-Client-Tests depend quite a lot on the new Mongo-Tests-ReplicaSet:

Screen Shot 2021-01-19 at 11 23 38

The reason is that even if MongoClient works on a scenario with a single mongod server, the central scenario has several mongod servers connected in a replica set. I don't know if it's worth to split in different packages also mongoclient in "single" vs "replica set" setup.

(Voyage-Mongo relies in MongoClient)

tinchodias commented 3 years ago

Anyway I'm trying to split Mongo-Client and Mongo-Client-Tests according to this dimension of "single" vs "replica set". I tell you later.

noha commented 3 years ago

Looks ok to me. The fact is not such much ignoring what is used or not. It is barely if the code is loadable. So refactoring tests is one thing, dependending on that group by default from voyage does not make sense because not every user is interested in the replicaset tests.

noha commented 3 years ago

So I wait with merging?

tinchodias commented 3 years ago

Yes, wait a bit more for merging. I'm analysing and sharing the thoughts because it's not simple and feedback from discussions help.

I understand that Voyage users may not have a replica set, but then what's the approach? we add a 'mongo replicaset tests' group?

I mean, current README in Voyage provides this script to isntall the VOMongoRepository:

Metacello new 
    repository: 'github://pharo-nosql/voyage/mc';
    baseline: 'Voyage';
    load: 'mongo tests'.

Do you mean to keep that as Voyage with MongoClient but for single servers. And add another separate entry for the MongoClient with replica set?

tinchodias commented 3 years ago

However, besides these noble efforts in being minimalistic by not depending on OSSubprocess... I briefly looked at other MongoDB bindings and I don't see any of them explaining how to install a single-server version of the binding instead of the full binding.

These, in particular: https://docs.mongodb.com/drivers/c#installation https://mongodb.github.io/mongo-java-driver/4.1/driver/getting-started/installation/ https://docs.mongodb.com/drivers/pymongo#installation https://docs.mongodb.com/drivers/go#installation https://docs.mongodb.com/drivers/node/quick-start#add-mongodb-as-a-dependency

I know that Pharo and Smalltalk are great and we can have better bindings than in other languages. But... Is it SO bad to have tests that depend other libraries?

BTW1: Maybe I didn't make this clear before... Our implementation of MongoClient had as reference other mongodb bindings (drivers). And what you can see in these other bindings is that you instance a client with 1..N initial urls and the client discovers what's the topology behind (single server/ replicaset / sharded) and abstracts the client user from it... the user mostly performs CRUD operations.

BTW2: anyway following in the idea of allowing some minimalism, we shouldn't forget that alternatively to splitting MongoClient into single vs replicaset, we may just change 'mongo tests' group in Voyage to load voyage tests but not any mongotalk tests.

noha commented 3 years ago

I think as long as you call it noble and use words like minimalism I get the impression you are reluctant to understand the impact of dependencies. Every dependency you can remove is a win. Complexity is what kills all software. Let's talk about dependencies. Do I need OSProcess in a production runtime? No quite the contrary. OSProcess is used only for testing the replica set. So we can agree that voyage does not load any tests from mongo and I would be fine. But I still find it better to have the core tests without dependency they don't need and move the dependency away so there is a degree of freedom. At the moment pharo9 users cannot use voyage and I'm not sure you think that is ok

tinchodias commented 3 years ago

:) Okay, maybe I could put less drama on those words. I think it's like this:

I also prefer to reduce dependencies in general and, while I'm doing efforts in this case, I see it wasn't so simple task. When I reconsider the situation, I find that the approach to fix this issue could be not that important. Then, I decided to express it on this issue-conversation to communicate with anybody interested and find a good fix for the issue. Also, trying to convince the other side of Martin Dias that wants to remove this dependency.

About the argument of production runtime... what are the downsides of installing packages without tests in that image? Couldn't be better idea to prepare it without tests?

tinchodias commented 3 years ago

I will revert the last commit in this branch because it's offtopic and I keep it for another PR. I understand that @noha approved merging so I do it and we can continue discussion in that "next PR".