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

[Discuss] Force definitions for shared features #505

Closed
wants to merge 1 commit into from
Closed

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jan 30, 2017

Features that share a name should share a definition. This:

  • avoids the risk that a format implements a feature that clashes with
    the name of another feature
  • avoids the risk of implementing the same feature in a slightly
    different way in multiple places
  • makes it harder to write details.json poorly

I've raised this PR to have a discussion before devoting more time to it. As part of template consolidation we are documenting the inconsistencies between content schemas and looking at ways to make it difficult to introduce further inconsistencies without impeding development.

Here is the output of the rake task on master which shows which duplicate features aren't using defintions:

The following features are used in multiple schemas without definitions:

'body'
  - formats/case_study/publisher/details.json
  - formats/consultation/publisher/details.json
  - formats/detailed_guide/publisher/details.json
  - formats/document_collection/publisher/details.json
  - formats/fatality_notice/publisher/details.json
  - formats/hmrc_manual/publisher/details.json
  - formats/hmrc_manual_section/publisher/details.json
  - formats/html_publication/publisher/details.json
  - formats/manual/publisher/details.json
  - formats/manual_section/publisher/details.json
  - formats/news_article/publisher/details.json
  - formats/publication/publisher/details.json
  - formats/service_manual_guide/publisher/details.json
  - formats/service_manual_service_standard/publisher/details.json
  - formats/specialist_document/publisher/details.json
  - formats/speech/publisher/details.json
  - formats/statistical_data_set/publisher/details.json
  - formats/take_part/publisher/details.json
  - formats/topical_event_about_page/publisher/details.json
  - formats/working_group/publisher/details.json
  - formats/world_location_news_article/publisher/details.json

'first_public_at'
  - formats/case_study/publisher/details.json
  - formats/consultation/publisher/details.json
  - formats/detailed_guide/publisher/details.json
  - formats/document_collection/publisher/details.json
  - formats/fatality_notice/publisher/details.json
  - formats/news_article/publisher/details.json
  - formats/publication/publisher/details.json
  - formats/speech/publisher/details.json
  - formats/statistical_data_set/publisher/details.json
  - formats/world_location_news_article/publisher/details.json

'change_note'
  - formats/case_study/publisher/details.json
  - formats/placeholder/publisher/details.json
  - formats/service_manual_guide/publisher/details.json

'tags'
  - formats/case_study/publisher/details.json
  - formats/consultation/publisher/details.json
  - formats/detailed_guide/publisher/details.json
  - formats/document_collection/publisher/details.json
  - formats/hmrc_manual/publisher/details.json
  - formats/news_article/publisher/details.json
  - formats/placeholder/publisher/details.json
  - formats/publication/publisher/details.json
  - formats/speech/publisher/details.json
  - formats/statistical_data_set/publisher/details.json
  - formats/world_location_news_article/publisher/details.json

'withdrawn_notice'
  - formats/case_study/publisher/details.json
  - formats/consultation/publisher/details.json
  - formats/detailed_guide/publisher/details.json
  - formats/document_collection/publisher/details.json
  - formats/publication/publisher/details.json
  - formats/service_manual_guide/publisher/details.json
  - formats/service_manual_topic/publisher/details.json
  - formats/statistical_data_set/publisher/details.json

'public_updated_at'
  - formats/coming_soon/publisher/details.json
  - formats/unpublishing/publisher/details.json

'documents'
  - formats/consultation/publisher/details.json
  - formats/publication/publisher/details.json

'summary'
  - formats/email_alert_signup/publisher/details.json
  - formats/finder/publisher/details.json
  - formats/policy/publisher/details.json
  - formats/travel_advice/publisher/details.json

'breadcrumbs'
  - formats/email_alert_signup/publisher/details.json
  - formats/hmrc_manual_section/publisher/details.json

'beta'
  - formats/finder/publisher/details.json
  - formats/finder_email_signup/publisher/details.json

'document_noun'
  - formats/finder/publisher/details.json
  - formats/policy/publisher/details.json

'default_documents_per_page'
  - formats/finder/publisher/details.json
  - formats/policy/publisher/details.json

'default_order'
  - formats/finder/publisher/details.json
  - formats/policy/publisher/details.json

'filter'
  - formats/finder/publisher/details.json
  - formats/policy/publisher/details.json

'facets'
  - formats/finder/publisher/details.json
  - formats/policy/publisher/details.json

'show_summaries'
  - formats/finder/publisher/details.json
  - formats/policy/publisher/details.json

'external_related_links'
  - formats/generic_with_external_related_links/publisher/details.json
  - formats/placeholder/publisher/details.json

'child_section_groups'
  - formats/hmrc_manual/publisher/details.json
  - formats/hmrc_manual_section/publisher/details.json
  - formats/manual/publisher/details.json

'change_notes'
  - formats/hmrc_manual/publisher/details.json
  - formats/manual/publisher/details.json

'organisations'
  - formats/hmrc_manual/publisher/details.json
  - formats/hmrc_manual_section/publisher/details.json
  - formats/manual/publisher/details.json
  - formats/manual_section/publisher/details.json

'manual'
  - formats/hmrc_manual_section/publisher/details.json
  - formats/manual_section/publisher/details.json

'internal_name'
  - formats/mainstream_browse_page/publisher/details.json
  - formats/policy/publisher/details.json
  - formats/taxon/publisher/details.json
  - formats/topic/publisher/details.json

'groups'
  - formats/mainstream_browse_page/publisher/details.json
  - formats/service_manual_topic/publisher/details.json
  - formats/topic/publisher/details.json

'attachments'
  - formats/manual_section/publisher/details.json
  - formats/specialist_document/publisher/details.json

'max_cache_time'
  - formats/specialist_document/publisher/details.json
  - formats/travel_advice/publisher/details.json
  - formats/travel_advice_index/publisher/details.json

'email_signup_link'
  - formats/travel_advice/publisher/details.json
  - formats/travel_advice_index/publisher/details.json

'publishing_request_id'
  - formats/travel_advice/publisher/details.json
  - formats/travel_advice_index/publisher/details.json
* Features that share a name should share a definition
* Avoids the risk that a format implements a feature that clashes with
the name of another feature
* Avoids the risk of implementing the same feature in a slightly
different way in multiple places
* Makes it harder to write `details.json` poorly
@tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Jan 30, 2017

This is incredibly useful! Great way to reduce duplication and drift between formats.

end
end

abort "\nThe following features are used in multiple schemas without definitions:\n\n" + definition_errors.join("\n") if definition_errors.any?

This comment has been minimized.

@tijmenb

tijmenb Jan 30, 2017
Contributor

Would field or attribute work here instead of feature? I've not seen that term used before in this context.

This comment has been minimized.

@fofr

fofr Jan 30, 2017
Author Contributor

Good suggestion.

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 31, 2017

Closing this PR as there appears to be broad support for this approach.
Will re-open with a cleaner rake task and commits to fix existing errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants