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

Documentation #19

Merged
merged 8 commits into from
Apr 22, 2019
Merged

Documentation #19

merged 8 commits into from
Apr 22, 2019

Conversation

dfreeman
Copy link
Member

I've been working on this in bits and pieces since completing the multi-package refactoring a couple of weeks ago, and finally had the chance to finish up an initial cut of a documentation site.

The docs include explanations of the concepts involved with milestones and some interactive examples to demonstrate them. A draft is up at https://salsify.github.io/milestones. I'd love feedback on:

  • whether the high level breakdown of topics makes sense
  • anything is confusing or poorly explained
  • anything is missing
  • there are typos, broken links, things like that

The diff in this PR is pretty big. I've broken it into commits that more or less make sense independently and can be reviewed on their own, but some of those are still pretty hefty. If in doubt, I'd much rather get feedback on the documentation content than the code, but getting eyes on both would of course be best.

/cc @salsify/frontend-developers

@dfreeman dfreeman requested a review from mnoble01 April 18, 2019 11:17
@dfreeman dfreeman mentioned this pull request Apr 18, 2019
3 tasks
@dfreeman dfreeman changed the base branch from rename-babel-plugin to master April 18, 2019 14:17
Copy link

@mnoble01 mnoble01 left a comment

Choose a reason for hiding this comment

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

I notice you did not include any examples of the .and* usage:

await advanceTo(MyMilestone).andContinue()

Is this because the alternative is easier to follow in the playground, or for some other reason?

@@ -0,0 +1,25 @@
# @milestones/ember

The `@milestones/ember` addon integrates with the Ember runtime and ember-cli to provide zero-config setup for working with milestones.
Copy link

Choose a reason for hiding this comment

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

Could you say something about what milestones addresses that is not covered by the Ember built-ins explained here?
https://guides.emberjs.com/release/testing/acceptance/

Maybe the reader can deduce from the Core docs, but I think it would help to be more explicit.
The official Ember docs imply, perhaps unintentionally, that the ember test helpers already provide all you need to test asynchronous code.

In a few Salsify projects we've used milestones heavily, but since it's not (yet) a widely used package, I've wondered how other teams tackle these problems. A mocking library like sinon? Simply not writing certain kinds of tests?

A couple sentences, or maybe a before/after example as in the ember-concurrency docs, could clarify when it's the right tool.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great point. I had a bit written up about this (and it was also the bulk of the content in the mini-talk I gave at the Boston meetup last fall), but it got lost in the shuffle as I was writing everything else up.

I'll re-integrate that into the Ember page on the site (my goal is to keep these READMEs as lightweight as possible and push people toward the richer interactive site), and I'll also try and add a bit of more general-purpose motivation to the core docs as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've wondered how other teams tackle these problems. A mocking library like sinon? Simply not writing certain kinds of tests?

Broadly speaking I think people tend to go one of three ways, depending on their personal priorities:

  • value coverage and reliability: sprinkle sleeps throughout code to always ensure you've settled in the right state, but accept that your test suite will be slow
  • value coverage and speed: make a best effort at ensuring state, but live with the fact that tests that pass in one environment may well fail in another
  • value speed and reliability: unit test what you can, and accept that there are some things you just don't have integration coverage for

I've done all of those at different points in the past, at Salsify and elsewhere, in codebases with and without Ember. My general experience is that projects tend to be in a constant state of flux between those different tradeoffs, depending on the values of the person who most recently 'fixed' the test suite.

@dfreeman
Copy link
Member Author

@mnoble01 @skarger Thanks for your reviews! I think I've integrated all the feedback you gave—the biggest change is introducing a more explicit Motivation section to the first 'real' page of the docs, and then addressing some Ember-specific notes relating to that on the Ember page.

I notice you did not include any examples of the .and* usage:

I wanted to avoid using that before it gets introduced on the Interacting with Milestones page, but aside from explicitly listing them there, a quick search shows a few instances on the Coordination, Ember and Milestone Keys pages. The new introductory example on the first page also has a .andReturn() in it (though since that appears ahead of the Interacting page and has a lot going on in it, it's explicitly called out as a "there are simpler examples ahead" situation).

Was there something specific with the .andX shorthand you were hoping to see that's not in there?

Copy link

@skarger skarger left a comment

Choose a reason for hiding this comment

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

Just a couple typos.

@mnoble01
Copy link

@dfreeman

Was there something specific with the .andX shorthand you were hoping to see that's not in there?

My mistake, looks like you do use the shorthand in a few examples. I would like to see the shorthand called out specifically.

You mention it in the API reference section for MilestoneTarget:

A MilestoneTarget also contains shortcut methods for easily invoking a particular behavior when the milestone is reached.

But the shorthand syntax is not listed anywhere. It is left to be inferred from scattered examples.

@dfreeman
Copy link
Member Author

@mnoble01
Copy link

@dfreeman Facepalm! It's covered

@dfreeman
Copy link
Member Author

Thanks again for taking the time to go through all this—I really appreciate it!

@dfreeman dfreeman merged commit 1cbdc15 into master Apr 22, 2019
@dfreeman dfreeman deleted the docs-site branch April 22, 2019 16:00
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