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 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.

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