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

Update Ricardian spec to version 0.2 #22

Merged
merged 6 commits into from
Jun 10, 2019
Merged

Update Ricardian spec to version 0.2 #22

merged 6 commits into from
Jun 10, 2019

Conversation

esheffield
Copy link
Contributor

Mainly adds a section defining the available helpers and which version
of the spec they became available in.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

* `amount_from_asset` - Given a variable containing an asset, extract and return the amount.

* `symbol_name_from_asset` - Given a variable containing an asset, extract and return the symbol name.
Copy link

Choose a reason for hiding this comment

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

Since this is redundant with asset_to_symbol_code can we mark this as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either one. Personally I prefer this one - the name seems more descriptive, and it's nicely symmetric with the amount_from_asset helper.

README.md Outdated Show resolved Hide resolved
README.md Outdated
In addition to the standard helpers which are part of Handlebars, there are a number of additional helpers defined to assist with Ricardian contract specific issues.

___Since 0.0___
* `wrap` - Takes no parameters. Block level helper to wrap the contents with `<div>` tags for highlighting. See `Variables` section above for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

for the Variables mentions here and below - might be helpful to link the header in the README, like so Variables

README.md Outdated

{{ asset_to_symbol_code asset }} --> 'EOS'.

___Since 0.1___
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these references to versions? If so, maybe __Since v0.1___ would be more clear here

asset = '2.001 EOS'

{{ amount_from_asset asset }} --> '2.001'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you might be missing a closing ``` here for the example above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL that VSCode markdown preview masks when closing ``` are missing.

README.md Outdated

{{ amount_from_asset asset }} --> '2.001'.

* `symbol_name_from_asset` - Given a variable containing an asset, extract and return the symbol name. For example: '2.001 EOS' --> returns 'EOS'.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also remove the example in this description (For example: '2.001 EOS' --> returns 'EOS'.) since you added it below

README.md Outdated
asset = '2.001 EOS'

{{ symbol_name_from_asset asset }} --> 'EOS'.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the closing ``` is missing here as well

Copy link
Contributor

@TaraTritt TaraTritt left a comment

Choose a reason for hiding this comment

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

Few comments else LGTM

@esheffield esheffield changed the base branch from master to develop June 7, 2019 14:49
Eddie Sheffield added 3 commits June 7, 2019 10:50
Mainly adds a section defining the available helpers and which version
of the spec they became available in.
Copy link

@brandonfancher brandonfancher left a comment

Choose a reason for hiding this comment

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

Mostly minor tweaks. But I am wondering if we can drop the deprecated helpers that we're documenting for the first time. Do we have any evidence anyone knew of their existence?

README.md Outdated
@@ -97,6 +97,115 @@ In cases where this default variable wrapping can cause problems (e.g. a variabl

`{{#wrap}}[{{nowrap link.text}}]({{nowrap link.url}}){{/wrap}}`

#### Handlebars helpers

Choose a reason for hiding this comment

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

We're using Title Case throughout; let's uppercase "helpers".

README.md Outdated
{{ else }}
Render if myVar is undefined or null
{{/if_has_value}}
```
#### HTML in variables

Choose a reason for hiding this comment

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

Let's go ahead and uppercase "variables" too.

README.md Outdated
auth = {
"actor": "alicejones",
"permission": "active"
}

Choose a reason for hiding this comment

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

Indentation could be improved here.

README.md Outdated

In Ricardian contract:
{{ permission_in_permission_level auth }} --> 'active'
```

Choose a reason for hiding this comment

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

Indentation could be improved here.

Choose a reason for hiding this comment

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

Because these two helpers are being documented for the first time and they're deprecated, can we just drop them?

README.md Outdated
{{ amount_from_asset asset }} --> '2.001'.
```

* `symbol_name_from_asset` - _(__Deprecated__ Use `asset_to_symbol_code` instead.)_ Given a variable containing an asset, extract and return the symbol name.

Choose a reason for hiding this comment

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

Same question for this... drop it?

@esheffield esheffield merged commit 60643da into develop Jun 10, 2019
@esheffield esheffield deleted the update-v0.2 branch June 10, 2019 15:37
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.

4 participants