Skip to content

Add class for highlighting, replacing and extracting placeholders#1

Merged
mannickutd merged 10 commits intoalphagov:masterfrom
quis:placeholders-library
Feb 17, 2016
Merged

Add class for highlighting, replacing and extracting placeholders#1
mannickutd merged 10 commits intoalphagov:masterfrom
quis:placeholders-library

Conversation

@quis
Copy link
Member

@quis quis commented Feb 16, 2016

Notes for reviewing

Reviewing will be easier on a commit-by-commit basis

I would particularly like to know:

  • if there are any test cases I might have missed

Usage

Given a template object, the Template class can:

  • highlight the placeholders (by adding HTML)
  • replace the placeholders with data
  • extract the names of the placeholders

It will also pass through, if passed the name and id of the template, and the user’s data, eg:

  template = Template(
    {"id": "1234", "name": "Hello world", "content": "Hello ((name))"},
    {"name": "chris"}
  )
  template.id
  >>> "1234"
  template.name
  >>> "Hello world"
  template.content
  >>> "Hello ((name))"
  template.values
  >>> {"name": "chris"}

If you only have the template

  template = Template({"content": "Hello ((name))"})

Highlight the placeholders:

  template.formatted
  >>> "Hello <span class='placeholder'>name</span>"
  template.formatted_as_markup
  >>> Markup(u"Hello <span class='placeholder'>name</span>")

List the placeholders in the template

  template.placeholders
  >>> ['name']
  template.placeholders_as_markup
  >>> [Markup(u"<span class='placeholder'>name</span>")]

If you have the template and the user’s data

You can do all of the above, plus

Replace the placeholders with the user’s data

  template = Template({"content": "Hello ((name))"}, {"name": "Chris"})
  template.replaced
  >>> 'Hello Chris'

Find out what data is missing for a given template

  template = Template({"content": "Hello ((name))}", {"foo": "bar"})
  template.missing_data
  >>> {'name'}

Find out what additional data is being passed that the template isn’t expecting

  template = Template({"content": "Hello ((name))"}, {"foo": "bar"})
  template.additional_data
  >>> {'foo'}

If you try to replace placeholders with bad data

…then you’ll get a NeededByTemplateError or a NoPlaceholderForDataError, eg

  template = Template({"content": "Hello ((name))"}, {"foo": "bar"})
  >>> NeededByTemplateError: name

@quis quis changed the title Placeholders library Add class for highlighting, replacing and extracting placeholders Feb 16, 2016
@quis quis force-pushed the placeholders-library branch from 43ed5ce to ebba054 Compare February 16, 2016 10:33

@property
def missing_data(self):
return set(self.list) - set(self.values.keys())
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't need to be enclosed in sets?
Look at pythons set operators :)
https://docs.python.org/3.5/library/stdtypes.html?highlight=set#set

quis added 7 commits February 16, 2016 23:42
This commit copies the placeholder code which is currently in the admin app
here:
https://github.com/alphagov/notifications-admin/blob/78fe2b463aeb5b00c2984439226977679e0b4382/app/__init__.py#L126-L152

It then adds tests for existing code, which do not exist in the admin app.

It makes two changes from the code in the admin app:
- return strings not a markup object (because this code will be more
  general-purpose than just front end rendering of placeholders)
- fix handling of templates which have placeholders inside brackets
  (eg `(((name)))`) by making a slight change to the regular expression
Takes the template as an argument to the constructor, rather than having
static methods that all take template as an argument.

Add __repr__ and __str__ for easier debugging.
Adds the option to have each placeholder formatted as markup, for use as
column headings, for example.

Since we’re importing Markup now anyway, we might as well add a method to return
the template with placeholders highlighted as a Markup object.
This will also you to determine:
- if there are placeholders in the template which aren’t in the user’s data
- if there are columns in the user’s data which are additional to the
  placeholders in the template
There are multiple methods which need to know the values in order to replace
placeholders or list missing ones, and passing the values multiple times if
possible.

Therefore it’s useful to pass the values only once. But modifying the state of
an object is best avoided if possible. So the best time to pass the values is
on instantiation.
In trying to use this class I found it clumsy, because it accepted one attribute
of a template object (`content`) but not the others (`name`, `id`).

This commit changes the class to accept an entire template object (with `name`
and `id` still optional). All attributes of the template are passed through, and
the additional properties available (eg `template.replaced`) stay the same,
although in some cases are renamed for clarity.

This commit also updates the documentation to match.
@quis quis force-pushed the placeholders-library branch from ebba054 to 386489a Compare February 17, 2016 09:33
@quis
Copy link
Member Author

quis commented Feb 17, 2016

@mannickutd You’re right, don’t need the wrapping sets, I’ve squashed the changes into c890df1.

Other changes I’ve made since:

Rename from Placeholders to Template

In trying to use this class I found it clumsy, because it accepted one attribute of a template object (content) but not the others (name, id).

This commit changes the class to accept an entire template object (with name and id still optional). All attributes of the template are passed through, and the additional properties available (eg template.replaced`) stay the same, although in some cases are renamed for clarity.

This commit also updates the documentation to match.

Add option to drop values from the user’s data before parsing template

A user’s uploaded data may well have fields which aren’t in the template, for example phone number wouldn’t be in the template, but would need to be in the CSV. You wouldn’t want to raise an error when giving the template this data.

This commit adds an option, when instantiating the Template object, to drop values from the user’s data before attempting to do any replacing of the template.

@quis quis force-pushed the placeholders-library branch from 386489a to b8087d9 Compare February 17, 2016 09:39
quis added 3 commits February 17, 2016 10:50
A user’s uploaded data may well have fields which aren’t in the template,
for example `phone number` wouldn’t be in the template, but would need to be
in the CSV. You wouldn’t want to raise an error when giving the template this
data.

This commit adds an option, when instantiating the `Template` object, to drop
values from the user’s data before attempting to do any replacing of the
template.
Minor version bump because new features (`Template` class) have been added,
but in a non-breaking way.
@quis quis force-pushed the placeholders-library branch from b8087d9 to 43545b0 Compare February 17, 2016 10:51
quis added a commit to alphagov/notifications-admin that referenced this pull request Feb 17, 2016
This commit brings in the `Template` util, added here:
alphagov/notifications-utils#1

It also does a fair bit of tidying up, which I’ve unfortunately squashed into
this one massive commit. The main change is moving 404 handling into the
templates dao, so that every view isn’t littered with `try: … except(HTTPError)`.

It also adds new features, in a prototypy sort of way, which are:
- download a prefilled example CSV
- show all the columns for your template on the 'check' page
quis added a commit to alphagov/notifications-admin that referenced this pull request Feb 17, 2016
This commit brings in the `Template` util, added here:
alphagov/notifications-utils#1

It also does a fair bit of tidying up, which I’ve unfortunately squashed into
this one massive commit. The main change is moving 404 handling into the
templates dao, so that every view isn’t littered with `try: … except(HTTPError)`.

It also adds new features, in a prototypy sort of way, which are:
- download a prefilled example CSV
- show all the columns for your template on the 'check' page
quis added a commit to alphagov/notifications-admin that referenced this pull request Feb 17, 2016
This commit brings in the `Template` util, added here:
alphagov/notifications-utils#1

It also does a fair bit of tidying up, which I’ve unfortunately squashed into
this one massive commit. The main change is moving 404 handling into the
templates dao, so that every view isn’t littered with `try: … except(HTTPError)`.

It also adds new features, in a prototypy sort of way, which are:
- download a prefilled example CSV
- show all the columns for your template on the 'check' page
quis added a commit to alphagov/notifications-admin that referenced this pull request Feb 17, 2016
This commit brings in the `Template` util, added here:
alphagov/notifications-utils#1

It also does a fair bit of tidying up, which I’ve unfortunately squashed into
this one massive commit. The main change is moving 404 handling into the
templates dao, so that every view isn’t littered with `try: … except(HTTPError)`.

It also adds new features, in a prototypy sort of way, which are:
- download a prefilled example CSV
- show all the columns for your template on the 'check' page
mannickutd added a commit that referenced this pull request Feb 17, 2016
Add class for highlighting, replacing and extracting placeholders
@mannickutd mannickutd merged commit db03c19 into alphagov:master Feb 17, 2016
quis added a commit to alphagov/notifications-admin that referenced this pull request Feb 18, 2016
This commit brings in the `Template` util, added here:
alphagov/notifications-utils#1

It also does a fair bit of tidying up, which I’ve unfortunately squashed into
this one massive commit. The main change is moving 404 handling into the
templates dao, so that every view isn’t littered with `try: … except(HTTPError)`.

It also adds new features, in a prototypy sort of way, which are:
- download a prefilled example CSV
- show all the columns for your template on the 'check' page
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