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

Fix #16 #37

Merged
merged 3 commits into from
Dec 3, 2024
Merged

Fix #16 #37

merged 3 commits into from
Dec 3, 2024

Conversation

Seb-sti1
Copy link
Contributor

This should fix #16

As Rick Lupton guessed in the issue b.item.deleted_length is non-zero when node_id is None.

{
  "CrdtGroupItem": {
    "deletedLength": 1,
    "id": "1:16",
    "left": "0:0",
    "parentId": "0:11",
    "right": "0:0"
  },
  "minVersion": 1,
  "version": 1
}

compared to the following when deletedLength == 0

{
    "CrdtGroupItem": {
        "deletedLength": 0,
        "id": "0:13",
        "left": "0:0",
        "parentId": "0:1",
        "right": "0:0",
        "value": {
            "nodeId": "0:11"
        }
    },
    "minVersion": 1,
    "version": 1
}

It fixes the parsing of my file mentioned in #32 (19 CrdtGroupItem where nodeId == None and deletedLength > 0).

@Azeirah could you test if it fixes it for you too?

@ricklupton
Copy link
Owner

Thanks for fixing this. Although they should come together, I think it would be better to check for node_id not being None, rather than checking for deleted length -- if that makes sense would you mind changing the test, and adding a line to the change log in the README?

@Azeirah
Copy link
Contributor

Azeirah commented Nov 23, 2024

This should fix #16

As Rick Lupton guessed in the issue b.item.deleted_length is non-zero when node_id is None.

{
  "CrdtGroupItem": {
    "deletedLength": 1,
    "id": "1:16",
    "left": "0:0",
    "parentId": "0:11",
    "right": "0:0"
  },
  "minVersion": 1,
  "version": 1
}

compared to the following when deletedLength == 0

{
    "CrdtGroupItem": {
        "deletedLength": 0,
        "id": "0:13",
        "left": "0:0",
        "parentId": "0:1",
        "right": "0:0",
        "value": {
            "nodeId": "0:11"
        }
    },
    "minVersion": 1,
    "version": 1
}

It fixes the parsing of my file mentioned in #32 (19 CrdtGroupItem where nodeId == None and deletedLength > 0).

@Azeirah could you test if it fixes it for you too?

I don't know what the original file was where the error was happening. But I have error monitoring set-up for a large amount of people using rmscene and remarks together. I am updating the service to rmscene 0.6.0 and will definitely open up a new issue if something unexpected comes up.

@Seb-sti1
Copy link
Contributor Author

Thanks for fixing this. Although they should come together, I think it would be better to check for node_id not being None, rather than checking for deleted length -- if that makes sense would you mind changing the test, and adding a line to the change log in the README?

Done !

@ricklupton ricklupton merged commit bb61d8a into ricklupton:main Dec 3, 2024
6 checks passed
@ricklupton
Copy link
Owner

Thanks!

@Seb-sti1 Seb-sti1 deleted the value-error branch December 5, 2024 15:51
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.

Node does not exist for SceneGroupItemBlock: None
3 participants