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

Remove special assert syntax #894

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove special assert syntax #894

wants to merge 1 commit into from

Conversation

bplatz
Copy link
Contributor

@bplatz bplatz commented Sep 19, 2024

This code appears to create new flakes for @type values (classes) when being read from commits.

To my knowledge with the latest SID changes, this isn't needed and isn't done with initial transactions. At one point we did need a flake for every IRI, which is what I assume this was put in for.

I believe this would have the effect of a different db state for the same data when originally transacted vs read from commit files - as the latter would include flakes for every class, and the former would not.

After removing this code, all tests pass - but definitely want more eyeballs in case I'm missing something.

For a large commit, this code does a decent amount of work, so will help performance by removing it as well.

@bplatz bplatz requested a review from a team September 19, 2024 18:16
Copy link
Contributor

@zonotope zonotope left a comment

Choose a reason for hiding this comment

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

This code looks like it's combining all the maps in an assert sequence with the same id in case you have something like {"@id" "ex:foo", "ex:friend" {"@id" "ex:bar", "ex:age" 27}}, and {"@id" "ex:bar", "ex:favColor" "green"} in the same sequence. i don't think we need this functionality either. since the tests pass, i say we get rid of it and deal with any issues when they come up.

👍🏾

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