Skip to content
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

op-based counter #141

Open
wants to merge 5 commits into
base: crdt-wip
Choose a base branch
from
Open

Conversation

ajani2001
Copy link

No description provided.

Copy link
Owner

@d-r-q d-r-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мелочь поправть, пожалуйста и мёржи в отдельную ветку

@@ -41,7 +42,9 @@ data class LogsDiff(
resolve(writesFromA[it]!!, writesFromB[it]!!)
}
}
return resolvingEavsByGid.values.map { RawEntity(it.first().gid, it) }
return resolvingEavsByGid
.filter { !it.value.isEmpty() }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Во-первых тут идея ругается, что надо it.value.isNotEmpty()

Во-вторых чёт мне эта фильтрация в целом не очень нравится, давай хотя бы коммент напишем откуда там пустые значения

В-третьих, чёт меня малёха смущает, что это приведёт к утере атрибута сущности, а тут вроде везде речь о сущностях целиком

@@ -68,6 +71,7 @@ internal fun lastWriterWinsResolve(resolveAttrName: (String) -> Attr<Any>?): (Li
// temporary dirty hack until crdt counter or custom resolution strategy support is implemented
attr == Instances.nextEid -> listOf((eavsFromA + eavsFromB).maxByOrNull { it.eav.value as Int }!!.eav)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это надо выкосить?

@@ -61,17 +63,17 @@ class Index(
}
}

fun addFacts(facts: List<Eav>): Index =
addFacts(facts as Iterable<Eav>)
fun addFacts(facts: List<Eav>, resolveAttr: (String) -> Attr<*>? = { null }): Index =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Может resolveAttr уже в поля засунуть, раз такая пьянка?

@d-r-q d-r-q changed the base branch from master to crdt-wip May 3, 2022 02:41
@d-r-q d-r-q marked this pull request as ready for review May 4, 2022 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants