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

Add ADR for removal of Smartdown #2053

Closed

Conversation

chrisroos
Copy link
Contributor

This documents our decision to remove Smartdown from the Smart Answers project.

See Documenting Architecture Decisions1 for some background of ADRs.

See presentation/adr-0012 and backdrop/adr-0013 for examples of ADR use
within GDS.

Expected user-facing changes

  • None

This documents our decision to remove Smartdown from the Smart Answers project.

See Documenting Architecture Decisions[1] for some background of ADRs.

See presentation/adr-001[2] and backdrop/adr-001[3] for examples of ADR use
within GDS.

[1]: http://thinkrelevance.com/blog/2011/11/15/documenting-architecture-decisions
[2]: https://github.com/openregister/presentation/blob/master/doc/arch/adr-001-remove-kafka.md
[3]: https://github.com/alphagov/backdrop/blob/master/doc/arch/adr-001-implementation-language.md
@chrisroos
Copy link
Contributor Author

I plan to revisit this but I'd appreciate any feedback you might have, @floehopper.

@floehopper
Copy link
Contributor

I'll come back to this again, but here are a couple of quick initial poorly-expressed thoughts while I think of them:

  • The state of many of the Ruby Smart Answers would've made it hard to convert them to Smartdown without extensive refactoring.
  • Adding new features to Smartdown would generally have needed extensions to the grammar and hence the parser. I think the cost of adding new features to Smartdown would've been relatively high, especially considering that Ruby Smart Answers already had all the functionality we needed (apart from multiple questions-per-page).

@chrisroos
Copy link
Contributor Author

Thanks, @floehopper. I've now incorporated your comments. Feel free to suggest any other amendments.

How does this look to you, @dsingleton? Are there any obvious gaps in the document?

@dsingleton
Copy link
Contributor

I've only had time to skim this, but i'd want to give some proper feedback before merging, so could you hold off a bit? Mostly reflecting the initial aims of Smartdown (eg, publishable/workflowable representation).

@chrisroos
Copy link
Contributor Author

Mostly reflecting the initial aims of Smartdown (eg, publishable/workflowable representation).

@dsingleton: Would this not be more appropriate in a separate ADR, possibly in the Smartdown repo?

@chrisroos
Copy link
Contributor Author

I've spoken to @dsingleton and we've agreed that documenting Smartdown should be in a separate ADR. I'm going to rebase this on master in preparation for getting it merged.

@chrisroos
Copy link
Contributor Author

@floehopper: Is there anything else you want to add to this or can I merge it?

@floehopper
Copy link
Contributor

I think you should go ahead and merge it. I think what you have is pretty comprehensive and we can always add stuff later.

@dsingleton
Copy link
Contributor

I'm happy to write up an ADR 0 for the addition of Smartdown (could you add a card to the trello and put my face on it? ta).

I've got a couple of small suggestions before merging. I'll add them now.


Smartdown made it easier for content designers to edit questions and outcome content, but was quite restrictive when it came to defining the rules of the Smart Answer (see the rules in [pay-leave-for-parents/partner_earned_more_than_lower_earnings_limit][spl-complicated-next-node-rules], for example).

Smartdown didn't match Ruby Smart Answers in terms of features. Adding support for these features to Smartdown would have required extensions to the grammar and hence the parser. The cost of this would've been relatively high, especially considering that Ruby Smart Answers already had all the functionality we needed (apart from multiple questions-per-page).
Copy link
Contributor

Choose a reason for hiding this comment

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

Ruby Smart Answers already had all the functionality we needed

I think it would be worth qualifying that one of the benefits/functionality of Smartdown was the intent for it to be pased over an API, allowing publishing workflow. The decision here (which I 👍 with) is to not attempt to solve that problem, and focus on making the Ruby format as simple and accessible as possible, on the assumption of high turnover of people working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think/hope I've partially addressed this by adding a sentence to the decision section in d3b19cd.

I'm less sure about mentioning that Smartdown was designed to be passed over an API. That feels like a candidate for the Smartdown ADR to me. What do you think?

@dsingleton
Copy link
Contributor

Added some comments about clarifying the intent of the ADR, it's good, but feels a bit focused on what and less of the why, which is the important bit to preserve.

@chrisroos
Copy link
Contributor Author

I'm happy to write up an ADR 0 for the addition of Smartdown (could you add a card to the trello and put my face on it? ta).

Done - https://trello.com/c/4QGJGBGQ/130-add-architecture-decision-record-adr-about-smartdown

@dsingleton made me realise that I was conflating the use of ERB
outcomes and the removal of Smartdown when I wrote this. I think the use
of ERB makes it easier for content designers to edit Smart Answer
outcomes, _not_ the removal of Smartdown.
@dsingleton pointed out that the use of "project" was confusing as it
could mean this application or the Smart Answers ecosystem. I think we
could argue that both have been simplified but it's certainly more
obvious that the Smart Answers app has.
Based on feedback from @dsingleton.
@chrisroos
Copy link
Contributor Author

@dsingleton: Thanks for the feedback! I've attempted to address it in the more recent commits and would appreciate you taking another look when you have a minute.

@chrisroos chrisroos changed the title WIP: Add ADR for removal of Smartdown Add ADR for removal of Smartdown Nov 25, 2015
chrisroos added a commit that referenced this pull request Nov 25, 2015
Closes PR #2053.

Manually merged from add-architecture-decision-record-about-smartdown-removal.
I wanted to update one of the commit messages but didn't want to lose the
discussion on the pull request, so I've manually merged it.
@chrisroos
Copy link
Contributor Author

I've merged this in 968867b. Any further discussion can happen as a new pull request.

I manually merged it (rather than rebasing on master and force-pushing to this branch) in order to retain the discussion in here.

@chrisroos chrisroos closed this Nov 25, 2015
@chrisroos chrisroos deleted the add-architecture-decision-record-about-smartdown branch November 25, 2015 10:47
@dsingleton
Copy link
Contributor

👍

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