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

General Tidy Up ahead of Sharing with Elixir Community! #11

Merged
merged 14 commits into from
Oct 17, 2018

Conversation

nelsonic
Copy link
Member

@nelsonic nelsonic commented Oct 16, 2018

@iteles please review the copy changes ahead of @Danwhy's update to the "How?" section.
Thanks! ✨

@codecov
Copy link

codecov bot commented Oct 16, 2018

Codecov Report

Merging #11 into master will increase coverage by 2.33%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #11      +/-   ##
==========================================
+ Coverage   56.75%   59.09%   +2.33%     
==========================================
  Files          11        8       -3     
  Lines          37       22      -15     
==========================================
- Hits           21       13       -8     
+ Misses         16        9       -7
Impacted Files Coverage Δ
test/support/channel_case.ex
test/support/conn_case.ex
test/support/data_case.ex

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3eb130e...81162d4. Read the comment docs.

@Danwhy
Copy link
Member

Danwhy commented Oct 17, 2018

@nelsonic There are a few things/improvements I came across when implementing https://github.com/dwyl/alog/ that I'd like to add to this guide before we share it, as well as some answers to @RobStallion's questions in an 'FAQ'. I won't have a chance to do it before Friday though

@nelsonic
Copy link
Member Author

@Danwhy sweet! 🍭
I will not alter the "How?" (implementation) Section
so you can do your thing on Friday afternoon or Monday morning. 📆

@nelsonic nelsonic assigned iteles and unassigned nelsonic Oct 17, 2018
@nelsonic
Copy link
Member Author

@iteles in light of @Danwhy's learnings while creating https://github.com/dwyl/alog
the "How?" section of this example/tutorial will be updated independently of this PR.

Please read the first few questions of the README.md of this branch:
https://github.com/dwyl/phoenix-ecto-append-only-log-example/tree/tidy-up-for-publication-issue%231

If you are happy with the ("beginners' mind" focussed) changes, please merge
so that Dan can work from master.
Thanks! ✨

@nelsonic nelsonic added enhancement New feature or request awaiting-review and removed chore in-progress labels Oct 17, 2018
@nelsonic nelsonic added this to To do in Nelson's List via automation Oct 17, 2018
@nelsonic nelsonic moved this from To do to Done in Nelson's List Oct 17, 2018
Copy link
Member

@iteles iteles left a comment

Choose a reason for hiding this comment

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

I've added a commit with some basic flow changes (commas, removing italics when they were too close together and made the reader flow difficult, etc), but I haven't added anything that's in my comments above.

Let me know if you'd like me to do that @nelsonic or just merge the PR and update later.

Thanks everyone, so informative!

README.md Outdated
to store your App's data makes it _much_ easier
to build the App because records are never modified,
history is preserved and can easily be referred to
i.e: you have built-in "history"/traceability, debug-ability, and usage stats!
Copy link
Member

Choose a reason for hiding this comment

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

This last line is the 3rd time we've said this and is repeated again in the paragraph below. Repetition definitely drives the point home and is necessary but I think the point is much better made in the paragraph below (and line 78), so I would suggest we remove this i.e line to prevent people from skipping the paragraph below when they see it is repetition (that was my instinct).

Copy link
Member Author

Choose a reason for hiding this comment

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

@iteles agreed. if we can cut down the repetition it will be a really good thing!

README.md Outdated
People who want to improve the _reliability_ of the product they are building.
Those who want to understand more ("advanced")
"distributed" application architecture
including the ability to (optionally/incrementally) use IPFS and/or Blockchain!
Copy link
Member

Choose a reason for hiding this comment

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

Suggest: "(optionally/incrementally) build on this by using IPFS and/or Blockchain in the future !" to clarify tutorial doesn't cover this

Copy link
Member Author

Choose a reason for hiding this comment

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

@iteles please feel free to update the README.md on this branch with this suggestion ...

["_Time Travelling Debugger_"](http://elm-lang.org/blog/time-travel-made-easy)
which is an _incredibly_ powerful way to understand and debug an App.
By using an Append-only Log for _all_ data stored by our Elixir/Phoenix Apps,
we get a "time-travelling debugger" and _complete_ "analytics" _built-in_!
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be confusing for 'beginners', particularly those who have gone on to look into elm's debugger.

What we're saying is that you have all the data so you can figure all of this out from the data (and you can do this in nice ways like the last step of the readme). What a beginner may read is that you get a nice little sidebar and some form of analytics UI 'built in' which could be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@iteles we will have a "nice sidebar" in the UI once I'm "done" with this example! 😮

@iteles iteles added question Further information is requested and removed in-review labels Oct 17, 2018
@iteles iteles assigned nelsonic and unassigned iteles Oct 17, 2018
@nelsonic nelsonic assigned iteles and unassigned nelsonic Oct 17, 2018
@iteles iteles added in-review and removed awaiting-review question Further information is requested labels Oct 17, 2018
@iteles iteles assigned nelsonic and iteles and unassigned iteles and nelsonic Oct 17, 2018
@iteles iteles merged commit febb640 into master Oct 17, 2018
@iteles iteles deleted the tidy-up-for-publication-issue#1 branch October 17, 2018 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in-review
Projects
No open projects
Nelson's List
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants