Skip to content

Conversation

@mielvds
Copy link
Collaborator

@mielvds mielvds commented Dec 23, 2025

@mielvds mielvds changed the base branch from master to exception-chaining December 23, 2025 13:41
@mielvds mielvds marked this pull request as draft December 23, 2025 13:42
@mielvds mielvds changed the base branch from exception-chaining to 220-rdf-canon December 24, 2025 08:16
@mielvds mielvds force-pushed the merge-prs branch 2 times, most recently from 66eec3b to cf8ec97 Compare December 24, 2025 09:12
@mielvds mielvds marked this pull request as ready for review January 5, 2026 15:40
@davidlehn
Copy link
Member

General:

  • I'm not sure this was the best approach to handle old issues and PRs. It's now very difficult to review, and difficult to handle one piece at a time.
  • I don't think old issues should be closed until the issue is actually fixed, or at least merged into main.

Dropped "keys" / logging:

  • The issue addressed by the on_key_dropped callback patch likely needs to be more involved. That was the path jsonld.js had to start. It then moved over to "safe mode" to handle that and many other situations that could cause problems when signing canonized output.
  • If I had gotten to implementing a pyld "safe mode", it would likely have taken the simple path of throwing errors aligned with the js ones in all the needed places. More flexibility may only be needed for developer tools, which could be future work.
  • Not sure what to suggest should be done here. This patch only handles one situation of many needed for a full "safe mode". An important one though.
  • Is debug logging is the best default? Why not a no-op? The spec does allow dropped properties so arguably that's not something that needs debugging. If dropped properties are always an error is up for debate.
  • On naming, I think "property" might be better than "key"? It aligns more with the spec. For the situation here, I think the js code is using a "invalid property" event / error code. But I understand the desire to match API naming with native data structures.

@mielvds
Copy link
Collaborator Author

mielvds commented Jan 7, 2026

Hi @davidlehn

General:

  • I'm not sure this was the best approach to handle old issues and PRs. It's now very difficult to review, and difficult to handle one piece at a time.
  • I don't think old issues should be closed until the issue is actually fixed, or at least merged into main.

I agree that this is not ideal and I admit I'm being pragmatic here. But this repo has too many open PR's and issues ranging from 2 to 9 years that could no longer be merged. So this is mostly housekeeping. Noted though: I'll keep the issues open until they are actually fixed and merged.

I selected and checked this set of issues carefully because they are about valid issues, include tests and partial fixes. All spec tests still pass like they did before. Except for the tests, all fixes were done in different commits, so I could make individual PR's is desired but that's more work for me :) All in all, I think the changes are not too big and reviewing should still be doable. Best is to start with checking the tests. I will add some more explanatory comments to help understand the original issue/fix.

Dropped "keys" / logging:

  • The issue addressed by the on_key_dropped callback patch likely needs to be more involved. That was the path jsonld.js had to start. It then moved over to "safe mode" to handle that and many other situations that could cause problems when signing canonized output.

I agree but could we handle this is a new issue? I suggest we keep this change and possibly shake things up in a new release. I'm interested in how jsonld.js handles this.

  • If I had gotten to implementing a pyld "safe mode", it would likely have taken the simple path of throwing errors aligned with the js ones in all the needed places. More flexibility may only be needed for developer tools, which could be future work.

This is a good approach. Can we place handlers (in line with on_key_dropped) where needed and implement a safe mode on top of that? This would also give developers more fine-grained control.

  • Not sure what to suggest should be done here. This patch only handles one situation of many needed for a full "safe mode". An important one though.

I suggest a new issue to discuss a new iteration. I can open this and summarize what has been said so far.

  • Is debug logging is the best default? Why not a no-op? The spec does allow dropped properties so arguably that's not something that needs debugging. If dropped properties are always an error is up for debate.

I actually agree, I just wanted to stay true to the original implementation. Introducing a logger is probably also something for a different PR. I'll patch this.

  • On naming, I think "property" might be better than "key"? It aligns more with the spec. For the situation here, I think the js code is using a "invalid property" event / error code. But I understand the desire to match API naming with native data structures.

Hmm good point; I'll make the change. An explanation about 'key' in the docs should suffice

@mielvds mielvds self-assigned this Jan 8, 2026
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.

3 participants