-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
93 support for SQLite #141
Conversation
…ode and also fixes 2 unrelated unit tests
ad0b441
to
c21dd99
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #141 +/- ##
===========================================
+ Coverage 56.67% 57.70% +1.02%
===========================================
Files 31 31
Lines 4157 4161 +4
===========================================
+ Hits 2356 2401 +45
+ Misses 1801 1760 -41 ☔ View full report in Codecov by Sentry. |
e19d4b6
to
3f36643
Compare
3f36643
to
0f75590
Compare
Stupid me I closed it |
@paulwilke careful padawan, the vergence in the force is so strong with you that GitHub buttons get clicked if you do not control the dark side well enough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all minor points commented in place
README.md
Outdated
--- | ||
|
||
## Description | ||
|
||
Mapless is a schema-less persistence framework supporting multiple backends and offering a user-friendly API. For instance, querying Mapless objects involves a common family of methods, and there's no need to declare accessors and mutators. See [examples below](#examples). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escribir es borrar -- siempre decia Leandro Caniglia
En la segunda linea podes sacar el "For instance," y se lee mejor.
README.md
Outdated
|
||
Mapless is a schema-less persistence framework supporting multiple backends and offering a user-friendly API. For instance, querying Mapless objects involves a common family of methods, and there's no need to declare accessors and mutators. See [examples below](#examples). | ||
|
||
Designed to be schema-less, Mapless eliminates the need for schema maintenance and avoids any Object-Relational Mapping requirements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tambien eliminaria el "Designed to be schema-less," -- ya lo dijiste en el parrafo anterior
README.md
Outdated
philosopherUser := User new | ||
person: philosopher; | ||
save. | ||
|
||
"Query for that user by ID and get its person instance" | ||
aristotle := (User findId: philosopherUser id) person. | ||
``` | ||
## Installation | ||
## Install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmhh "How to Install" o "Installation" me suenan mejor, no se.
@@ -1,25 +1,34 @@ | |||
![Mapless](./header.png) | |||
![Mapless](./hero.jpg) | |||
|
|||
# Mapless | |||
|
|||
Schema-less persistence for Smalltalk with support for multiple backends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me gusto el tag de la imagen "Obsenly simple object persistence" -- schema-less and offering support for multiple backends
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgarau no te había preguntado pero justo estaba hablando sobre la tagline con @eMaringolo hace un ratito :)
|
||
spec baseline: 'MongoTalk' with: [ | ||
spec | ||
repository: 'github://pharo-nosql/mongotalk:v2.0/mc'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To discuss why a specific version is needed here (v2.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgarau as Mapless is used in production, all its dependencies need to be version pinned. Actually you reminded me we can pin a couple more! Thanks for commenting about it.
PS: MongoTalk 2.0 is needed for accessing MongoDB from Pharo 10 and 10
id := sqliteRow first. | ||
maplessData := Json readFrom: sqliteRow values second readStream. | ||
maplessData at: aMaplessRepository idPropertyName put: id. | ||
^ Mapless fromJSONObject: maplessData in: aMaplessRepository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering where the readStream would be closed.
if the readStream is on a String, then ignore the above comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgarau yes, the Json readFrom:
is the only user of that stream
^ String | ||
streamContents: [ :stream | | ||
stream | ||
<< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure where the <<
is implemented -- looks a bit confusing to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgarau it's a Pharo way to say nextPutAll:
:)
] | ||
|
||
{ #category : #accessing } | ||
MaplessSQLitePool >> currentClientDynamicVariable [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh.. wouldn't it be better currentClientDynamicVariableClass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgarau yes it would! great catch
nextPutAll: | ||
'(' , self busyClients size asString , ' busy, ' | ||
, self idleClients size asString , ' idle @' | ||
, databaseFilenameOrKeyword , ')' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you have a stream, I would rather avoid concatenation and use cascading nextPutAll:
messages
"Removes any client that might be expired or unreacheable." | ||
|
||
self busyClients copy do: [ :e | self ifUnavailablePurge: e ]. | ||
self idleClients copy do: [ :e | self ifUnavailablePurge: e ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why copying the collection when you operate on the elements?
would this work?
(self busyClients , self idleClients) do: [:e | self ifUnavailablePurge: e].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgarau because ifUnavailablePurge:
mutates the collection and you should never mutate a collection while iterating it?
* 93 support for SQLite (#141) * Adjusts the baseline * WIP for testSimple * Comment improvment * testSimpleSave green * fixed more tests * fixes benchmark for sqlite * makes unqlite benchmarks work and using JSON for serialization * changelog entry * fixes findAll: * fixes SQLite dependency in the baseline * fixes escaping single quote for insert and update, removes obsolete code and also fixes 2 unrelated unit tests * removes unused code and adds class comments * working well with SQLite ^3.38 * adds testTruncate * adds truncateAll * adds databaseStartOn: double dispatching with sqlLiteDatabaseStart * updates method for Pharo 10 to run tests * upgrades MongoTalk * adds Pharo64-10 and drops Pharo 7 * milliseconds to milliSeconds * updates changelog, readme and CI * adds missing , * wider range of acceptance for the tests of the replica set load balancer * adjust unit test from Pharo 11 * Run CI only on push * remove unused method and poorly worded comments * covering #drop * covers existence detection * removes unused method * covers insert: * adds coverage to findAll: aMaplessClass where: someConditions limit: aLimitOrNil sort: * readme edit * instance creation convenience * readme edit * baseline default now is to load Memory and SQLite backends * readme edit * readme edit * renames currentClientDynamicVariable to currentClientDynamicVariableClass --------- Co-authored-by: Sebastian Sastre <[email protected]> * changelog edit --------- Co-authored-by: Sebastian Sastre <[email protected]>
This PR update Mapless so it can be used in Pharo 11 and Pharo 10 making sure the CI is happy and coverage/regression detection is preserved.
As the changelog says:
Also reviewed the readme so more people can try it and get started.