EdnError 1.0.0 (std) pub API

[Grinkers]

Draft so we can bikeshed https://github.com/naomijub/edn-rs/issues/119

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (fe6b043) 71.38% compared to head (2fdd97e) 72.07%. Report is 1 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #120 +/- ## ========================================== + Coverage 71.38% 72.07% +0.69% ========================================== Files 8 8 Lines 940 931 -9 ========================================== Hits 671 671 + Misses 269 260 -9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Grinkers]

@naomijub Can we have branches allow --force? Just tried rebasing to keep things clean/linear

remote: error: GH013: Repository rule violations found for refs/heads/pub-api.        
remote: Review all repository rules at http://github.com/naomijub/edn-rs/rules?ref=refs%2Fheads%2Fpub-api        
remote: 
remote: - Cannot force-push to this branch        
remote: 
remote: - Changes must be made through a pull request.        
remote: 
To github.com:naomijub/edn-rs.git
 ! [remote rejected] pub-api -> pub-api (push declined due to repository rule violations)
error: failed to push some refs to 'github.com:naomijub/edn-rs.git'

[naomijub]

Will fix that once I get home

[naomijub]

Sorry I took this long, did not see the notification

[Grinkers]

Continuation from https://github.com/naomijub/edn-rs/pull/108

@naomijub Any result of namespaced maps? If not I think I'll remove NamespacedMap? I can't find any references.

[naomijub]

Do we not plan to support anything like ::namespace{:key "value}?

[Grinkers]

Do we not plan to support anything like ::namespace{:key "value}?

As far as I am aware, this is not valid EDN?

[naomijub]

Weird that works in clojure. What about feature it?

[Grinkers]

I'm going through tests now, to try and decouple integration tests vs unit tests. Especially for feature organization and ability to change internal structure without everything blowing up.

Here's a couple discrepancies I've found so far, with tags and NamespaceMaps

(clojure.edn/read-string "#domain/model (1 2 3)")
java.lang.RuntimeException: No reader function for tag domain/model [at <repl>:1:1]

(clojure.edn/read-string "{:key \"value\"}")
{:key "value"}

(clojure.edn/read-string "::namespace{:key \"value\"}")
java.lang.RuntimeException: Invalid token: ::namespace [at <repl>:5:1]

(clojure.edn/read-string "::namespace {:key \"value\"}")
java.lang.RuntimeException: Invalid token: ::namespace [at <repl>:6:1]

I'm not nearly as familiar with clojure as rust or other low level stuff, so maybe I'm missing something.

[Grinkers]

Still can't --force push to keep things tidy. I pushed on my fork for now, but once fixed I'll force-push here and we don't lose the discussion.

https://github.com/naomijub/edn-rs/compare/master...Grinkers:edn-rs:pub-api

Everything compiles and tests pass, but there's clearly still some todo!(). Doc tests were a little weird because some of the string based errors have backticks, which throw a mess in the doc macros. Just changed it to a print using Debug. Error now doesn't have Eq or PartialEq, as users shouldn't be creating and comparing errors with no standard string formatting.

@naomijub I need to revert NamespacedMaps if it's a proper thing. I suspect it must be some domain-specific clojure library or custom reader? I've never seen anything like it, so I'll need references

I'm also not sure about Tagged. The use case in clojure is for user supplied reader functions, but there's no way for users to supply their own custom data readers at runtime. I suspect you probably quickly end up needing to reimplement all of clojure, reader, and macros. I think we keep the Tagged type so users can do whatever but not support the Extensible part of EDN. No data is lost, if rust ends up just being a transport.

[naomijub]

Push force is allowed, should be allowed in branches, not sure what has happened

[naomijub]

I'm going through tests now, to try and decouple integration tests vs unit tests. Especially for feature organization and ability to change internal structure without everything blowing up.

Here's a couple discrepancies I've found so far, with tags and NamespaceMaps

(clojure.edn/read-string "#domain/model (1 2 3)")
java.lang.RuntimeException: No reader function for tag domain/model [at <repl>:1:1]

(clojure.edn/read-string "{:key \"value\"}")
{:key "value"}

(clojure.edn/read-string "::namespace{:key \"value\"}")
java.lang.RuntimeException: Invalid token: ::namespace [at <repl>:5:1]

(clojure.edn/read-string "::namespace {:key \"value\"}")
java.lang.RuntimeException: Invalid token: ::namespace [at <repl>:6:1]

I'm not nearly as familiar with clojure as rust or other low level stuff, so maybe I'm missing something.

I'm convinced, good to remove.

Also, ok on the tagged portion.

[Grinkers]

Push force is allowed, should be allowed in branches, not sure what has happened

remote: Resolving deltas: 100% (33/33), completed with 17 local objects.        
remote: error: GH013: Repository rule violations found for refs/heads/pub-api.        
remote: Review all repository rules at http://github.com/naomijub/edn-rs/rules?ref=refs%2Fheads%2Fpub-api        
remote: 
remote: - Changes must be made through a pull request.        
remote: 
To github.com:naomijub/edn-rs.git
 ! [remote rejected] pub-api -> pub-api (push declined due to repository rule violations)
error: failed to push some refs to 'github.com:naomijub/edn-rs.git'

Error is a bit different now. I think we need to allow changes not through a pull request

[naomijub]

All rulesets are disabled, we need to standardize which ruleset we want

[Grinkers]

That seemed to work. I think everything for master and nothing for branches would be nice? I don't expect a lot of conflicting stuff. Or I can just keep my mess elsewhere.

Still todo!()s as mentioned before. However take a look at cargo doc and see if the high level view of the public api seems good. Error is the biggest change. Followed by json_to_edn, and then removed things.

I still consider this a draft, but it's getting close for bundling up breaking changes.

[naomijub]

Tomorrow I might have some time to review. @evaporei can you help as well?

[Grinkers]

Renamed this draft to be for the EdnError rework. I'll start breaking things apart so it's easier to review/track. I'll rebase cleanly before this leaves being draft.

[Grinkers]

Closing due to discussion and commits being all over the place. Creating a new separate branch/PR to match the issue #119.

[evaporei]

Sorry I was away for a bit, I can definitely help with the review @naomijub

[Grinkers]

Sorry I was away for a bit, I can definitely help with the review @naomijub

121 and #122 are the cleaned up versions. #123 hopefully concluding all the breaking api changes