- From: Domenic Denicola <notifications@github.com>
- Date: Tue, 01 Aug 2017 03:41:33 +0000 (UTC)
- To: w3c/payment-request <payment-request@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/payment-request/pull/567/review/53395121@github.com>
domenic requested changes on this pull request.
Great simplification and consolidation. Some issues with the algorithm prose though.
> <var>details</var>.<a>total</a>.<a data-lt=
- "PaymentItem.amount">amount</a>.<a data-lt=
- "PaymentCurrencyAmount.value">value</a> is U+002D HYPHEN-MINUS,
- then <a>throw</a> a <a>TypeError</a>, optionally informing the
- developer that the total can't be negative.
+ "PaymentItem.amount">amount</a>.
+ </li>
+ <li>
+ <a>Check and canonicalize <var>total</var></a>. Rethrow any
I'm a little weirded out by how the variable name is both a variable name and part of the algorithm name. IMO "[Check and canonicalize the total] _total_" would be better.
> <var>details</var>.<a>total</a>.<a data-lt=
- "PaymentItem.amount">amount</a>.<a data-lt=
- "PaymentCurrencyAmount.value">value</a> is U+002D HYPHEN-MINUS,
- then <a>throw</a> a <a>TypeError</a>, optionally informing the
- developer that the total can't be negative.
+ "PaymentItem.amount">amount</a>.
+ </li>
+ <li>
+ <a>Check and canonicalize <var>total</var></a>. Rethrow any
+ exceptions.
In theory rethrowing exceptions is not necessary (https://infra.spec.whatwg.org/#algorithm-control-flow), but in practice we should stay consistent, and I believe the spec mostly includes this verbiage right now. So, just noting FYI.
> <var>details</var>.<a>total</a>.<a data-lt=
- "PaymentItem.amount">amount</a>.<a data-lt=
- "PaymentCurrencyAmount.value">value</a> is U+002D HYPHEN-MINUS,
- then <a>throw</a> a <a>TypeError</a>, optionally informing the
- developer that the total can't be negative.
+ "PaymentItem.amount">amount</a>.
+ </li>
+ <li>
+ <a>Check and canonicalize <var>total</var></a>. Rethrow any
This would also allow you to avoid the intermediate variables that have been inserted in various places.
> @@ -866,8 +827,8 @@
<var>paymentMethod</var> tuple.
</li>
<li>Let <var>data</var> be the result of <a data-cite=
- "!ECMA-262-2015#sec-json.parse">JSON-parsing</a> the second
- element in <var>paymentMethod</var> tuple.
+ "!ECMASCRIPT#sec-json.parse">JSON-parsing</a> the second element
+ in <var>paymentMethod</var> tuple.
While here: "**item** in **the** paymentMethod tuple"
> - <a>currency</a> can be any string that is valid within the currency
- system indicated by <a>currencySystem</a>.
+ <p>
+ A string containing a currency identifier. The value of
+ <a>currency</a> can be any string that is valid within the currency
+ system indicated by <a>currencySystem</a>.
+ </p>
+ <p>
+ When using [[!ISO4217]], all <a data-cite=
+ "!ecma-402#sec-iswellformedcurrencycode">well-formed</a> 3-letter
+ currency codes are allowed. Their canonical form is upper case.
+ However, the set of combinations of currency code for which
+ localized currency symbols are available is implementation
+ dependent. Where a localized currency symbol is not available, a
+ user agent SHOULD us the [[!ISO4217]] currency code SHOULD be used
+ for formatting.
Last sentence is messed up; should be either "a user agent should" or "should be used", not both.
> </pre>
+ <section>
+ <h3>
+ Validity checkers
+ </h3>
+ <p>
+ A <dfn data-cite="INFRA#javascript-string">JavaScript string</dfn> is
+ a <dfn>valid decimal monetary value</dfn> if it consists of the
+ following <dfn data-lt="code point" data-cite="INFRA#code-point">code
+ points</dfn> in the given order: [[!INFRA]]
+ </p>
+ <ol>
+ <li>Optionally, a single U+002D HYPHEN-MINUS (-), to indicate that
+ the amount is negative.
In Infra we've moved toward omitting the spelled out name, so you'd just do `U+002D (-)` these days. See https://infra.spec.whatwg.org/#code-points
> + </ol>
+ <div class="note">
+ The following regular expression is an implementation of the above
+ definition.
+ <pre class="hljs">^-?[0-9]+(\.[0-9]+)?$</pre>
+ </div>
+ <p>
+ To <dfn data-lt="check and canonicalize amount">check and
+ canonicalize <var>amount</var> <a>PaymentCurrencyAmount</a></dfn>,
+ run the following steps:
+ </p>
+ <ol data-link-for="PaymentCurrencyAmount">
+ <li>If <a>currencySystem</a> is not
+ <code>urn:iso:std:iso:4217</code>, terminate this algorithm.
+ </li>
+ <li>let <var>isValidCurrency</var> be the result of calling
Uppercase "let"
> + </div>
+ <p>
+ To <dfn data-lt="check and canonicalize amount">check and
+ canonicalize <var>amount</var> <a>PaymentCurrencyAmount</a></dfn>,
+ run the following steps:
+ </p>
+ <ol data-link-for="PaymentCurrencyAmount">
+ <li>If <a>currencySystem</a> is not
+ <code>urn:iso:std:iso:4217</code>, terminate this algorithm.
+ </li>
+ <li>let <var>isValidCurrency</var> be the result of calling
+ <a data-cite=
+ "!ecma-402#sec-iswellformedcurrencycode">IsWellFormedCurrencyCode</a>
+ abstract operation with <a>currency</a> as the argument.
+ </li>
+ <li>If <var>isValidCurrency</var> returns false, then throw a
s/returns/is/
> + <a>code points</a> in the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT
+ NINE (9).
+ </li>
+ </ol>
+ <div class="note">
+ The following regular expression is an implementation of the above
+ definition.
+ <pre class="hljs">^-?[0-9]+(\.[0-9]+)?$</pre>
+ </div>
+ <p>
+ To <dfn data-lt="check and canonicalize amount">check and
+ canonicalize <var>amount</var> <a>PaymentCurrencyAmount</a></dfn>,
+ run the following steps:
+ </p>
+ <ol data-link-for="PaymentCurrencyAmount">
+ <li>If <a>currencySystem</a> is not
It should be _amount_.currencySystem. Same for `currency` and "_amount_'s `value`" below.
> + NINE (9).
+ </li>
+ </ol>
+ <div class="note">
+ The following regular expression is an implementation of the above
+ definition.
+ <pre class="hljs">^-?[0-9]+(\.[0-9]+)?$</pre>
+ </div>
+ <p>
+ To <dfn data-lt="check and canonicalize amount">check and
+ canonicalize <var>amount</var> <a>PaymentCurrencyAmount</a></dfn>,
+ run the following steps:
+ </p>
+ <ol data-link-for="PaymentCurrencyAmount">
+ <li>If <a>currencySystem</a> is not
+ <code>urn:iso:std:iso:4217</code>, terminate this algorithm.
This is a string, so needs quotes around it.
> + <a data-cite=
+ "!ecma-402#sec-iswellformedcurrencycode">IsWellFormedCurrencyCode</a>
+ abstract operation with <a>currency</a> as the argument.
+ </li>
+ <li>If <var>isValidCurrency</var> returns false, then throw a
+ <a>RangeError</a> exception and terminate this algorithm, optionally
+ informing the developer that the currency is invalid.
+ </li>
+ <li>Let <var>value</var> be <var>amount</var>'s <a>value</a>.
+ </li>
+ <li>If <var>value</var> is not a <a>valid decimal monetary value</a>,
+ throw a <a>TypeError</a>, optionally informing the developer that the
+ currency is invalid.
+ </li>
+ <li>Canonicalize <a>currency</a> by converting it to upper case.
+ </li>
This is written a little ambiguously. I'd prefer "Set _amount_.currency to the result of [ASCII uppercasing](https://infra.spec.whatwg.org/#ascii-uppercase) _amount_.currency". (Or is ASCII uppercasing incorrect here? In which case we should pick a well-defined uppercasing algorithm.)
> + </ol>
+ <p>
+ To <dfn data-lt="check and canonicalize total">check and canonicalize
+ <var>total</var> <a>PaymentCurrencyAmount</a></dfn>, run the
+ following steps:
+ </p>
+ <ol data-link-for="PaymentCurrencyAmount">
+ <li>If <a>currencySystem</a> is not
+ <code>urn:iso:std:iso:4217</code>, terminate this algorithm.
+ </li>
+ <li>
+ <a>Check and canonicalize <var>amount</var></a>. Rethrow any
+ exceptions.
+ </li>
+ <li>If the first <a>code point</a> of <var>value</var> is U+002D
+ HYPHEN-MINUS, then throw a <a>TypeError</a> optionally informing the
RangeError seems better? But maybe not worth the churn at this stage.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/payment-request/pull/567#pullrequestreview-53395121
Received on Tuesday, 1 August 2017 03:42:27 UTC