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

Expanded mutation interface. Created pessimistic mutation ns. Added more docs. #4

Closed
wants to merge 1 commit into from

Conversation

awkay
Copy link
Contributor

@awkay awkay commented Oct 20, 2018

No description provided.

@awkay
Copy link
Contributor Author

awkay commented Oct 20, 2018

Wilker, I'd appreciate you glancing at the new pessimistic mutation ns and seeing if it needs anthying else before I release 0.0.3...in particular if it needs more renames of any kind in order to make it ok.

I did a lot of name normalizing to follow fulcro standards...I think I got em all.

Copy link
Collaborator

@wilkerlucio wilkerlucio left a comment

Choose a reason for hiding this comment

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

Very nice additions, I like how we don't need a separated macro anymore 👍

Just left a few comments about the names, using an unqualified keyword to point an error seems prone to collision IMO.

README.adoc Outdated

```
:fulcro.incubator.pessimistic-mutations/mutation-response {
:error {:fulcro.client.primitives/error #error {:message "You screwed up!!!", :data {}}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe this should be

Suggested change
:error {:fulcro.client.primitives/error #error {:message "You screwed up!!!", :data {}}}
:fulcro.incubator.pessimistic-mutations/error {:fulcro.client.primitives/error #error {:message "You screwed up!!!", :data {}}}

Copy link
Contributor Author

@awkay awkay Oct 20, 2018

Choose a reason for hiding this comment

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

It's in embedded key in a map already keyed by a namespaced keyword...seems like overkill to me. In fact, I'd like to clean these maps up quite a bit and have much clearer meanings for things, and probably drop all nses on the internal bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait, I see what you mean. This is the response from the mutation...

README.adoc Outdated
```

The main one to look for is the last one, which will not change. The other keys are still under refinement, but currently
the actual network error from the server (when an `ex-info` is thrown) is in the nested `:error` map.
Copy link
Collaborator

Choose a reason for hiding this comment

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

then update here too if you agree

@awkay awkay closed this Oct 22, 2018
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