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

Editorial: Refactor to replace camelCase slot and field names with PascalCase equivalents #770

Closed
wants to merge 3 commits into from

Conversation

ben-allen
Copy link
Contributor

fix #81

Changed slot/field names, carefully avoiding changes to observable behavior that could result from dynamic accesses to those slots. The only names left unchanged are two-letter names like [[co]] and [[nu]], and slots/fields only accessed dynamically.

equivalent, carefully avoiding changes to observable behaviour which
might be thereby generated.
Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

This is better than the previous attempt, but is still running into similar issues. I think it will make the most sense to tackle fields piecemeal, leaving those subject to [[<_…>]] dynamic lookup for last.

Comment on lines 130 to 137
1. For each row in <emu-xref href="#table-datetimeformat-components"></emu-xref>, except the header row, in table order, do
1. Let _prop_ be the name given in the Property column of the row.
1. If _bestFormat_ has a field [[&lt;_prop_&gt;]], then
1. Let _p_ be _bestFormat_.[[&lt;_prop_&gt;]].
1. Let _first_ be the ASCII-uppercase of the substring of _prop_ from 0 to 1.
1. Let _rest_ be the substring of prop from 1.
1. Let _field_ be the string-concatenation of _first_ and _rest_.
1. If _bestFormat_ has a field [[&lt;_field_&gt;]], then
1. Let _p_ be _bestFormat_.[[&lt;_field_&gt;]].
1. Set _dateTimeFormat_'s internal slot whose name is the Internal Slot column of the row to _p_.
Copy link
Contributor

Choose a reason for hiding this comment

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

This new string manipulation and case-variant fallback makes me nervous, but I think it can now be dropped entirely (assuming field name capitalization and corresponding updates to DateTimeStyleFormat and BasicFormatMatcher and BestFitFormatMatcher.

Suggested change
1. For each row in <emu-xref href="#table-datetimeformat-components"></emu-xref>, except the header row, in table order, do
1. Let _prop_ be the name given in the Property column of the row.
1. If _bestFormat_ has a field [[&lt;_prop_&gt;]], then
1. Let _p_ be _bestFormat_.[[&lt;_prop_&gt;]].
1. Let _first_ be the ASCII-uppercase of the substring of _prop_ from 0 to 1.
1. Let _rest_ be the substring of prop from 1.
1. Let _field_ be the string-concatenation of _first_ and _rest_.
1. If _bestFormat_ has a field [[&lt;_field_&gt;]], then
1. Let _p_ be _bestFormat_.[[&lt;_field_&gt;]].
1. Set _dateTimeFormat_'s internal slot whose name is the Internal Slot column of the row to _p_.
1. For each row in <emu-xref href="#table-datetimeformat-components"></emu-xref>, except the header row, in table order, do
1. Let _name_ be the name given in the Internal Slot/Field column of the row.
1. If _bestFormat_ has a field whose name is _name_, then
1. Let _val_ be the value of the field of _bestFormat_ whose name is _name_.
1. Set _dateTimeFormat_'s internal slot whose name is _name_ to _val_.

spec/datetimeformat.html Outdated Show resolved Hide resolved
Comment on lines -226 to +234
<li>A [[pattern]] field, whose value is a String value that contains for each of the date and time format component fields of the record a substring starting with *"{"*, followed by the name of the field, followed by *"}"*.</li>
<li>If the record has an [[hour]] field, it must also have a [[pattern12]] field, whose value is a String value that, in addition to the substrings of the [[pattern]] field, contains at least one of the substrings *"{ampm}"* or *"{dayPeriod}"*.</li>
<li>If the record has a [[year]] field, the [[pattern]] and [[pattern12]] values may contain the substrings *"{yearName}"* and *"{relatedYear}"*.</li>
<li>A [[Pattern]] field, whose value is a String value that contains for each of the date and time format component fields of the record a substring starting with *"{"*, followed by the name of the field, followed by *"}"*.</li>
<li>If the record has an [[Hour]] field, it must also have a [[Pattern12]] field, whose value is a String value that, in addition to the substrings of the [[Pattern]] field, contains at least one of the substrings *"{ampm}"* or *"{dayPeriod}"*.</li>
<li>If the record has a [[Year]] field, the [[Pattern]] and [[Pattern12]] values may contain the substrings *"{yearName}"* and *"{relatedYear}"*.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, capitalizing the field names in [[LocaleData]].[[<locale>]].[[Formats]].[[<calendar>]] disrupts this and related mechanisms because of the coupling between field name and brace-enclosed field value substrings (e.g., an "{hour}:{minute} {ampm}" pattern is specifically referencing fields named [[hour]], [[minute]], and [[ampm]]). It's probably most expedient to leave alone any field named after a unit/date–time element/etc., unless you want to do a deeper dive on the format patterns and FormatDateTimePattern's transformation of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

blast, I should have caught that! I'm guessing it's correct to assume that a deeper dive on FormatDateTimePattern should be understood as a very low priority.

<li>If the record has an [[hour]] field, it must also have a [[rangePatterns12]] field. Its value is similar to the Record in [[rangePatterns]], but it uses a String similar to [[pattern12]] for each part of the range pattern.</li>
<li>If the record has a [[year]] field, the [[rangePatterns]] and [[rangePatterns12]] fields may contain range patterns where the [[Pattern]] values may contain the substrings *"{yearName}"* and *"{relatedYear}"*.</li>
<li>If the record has an [[Hour]] field, it must also have a [[RangePatterns12]] field. Its value is similar to the Record in [[RangePatterns]], but it uses a String similar to [[Pattern12]] for each part of the range pattern.</li>
<li>If the record has a [[Year]] field, the [[RangePatterns]] and [[RangePatterns12]] fields may contain range patterns where the [[Pattern]] values may contain the substrings *"{yearName}"* and *"{relatedYear}"*.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an example of the inconsistency; [[Year]] and {yearName} and {relatedYear} are elements of the same namespace even though they don't look like it.

Comment on lines -791 to +800
1. Let _timeFormat_ be _styles_.[[TimeFormat]].[[&lt;_timeStyle_&gt;]].
1. Let _first_ be the ASCII-uppercase of the substring of _timeStyle_ from 0 to 1.
1. Let _rest_ be the substring of _timeStyle_ from 1.
1. Let _timeStyleField_ be the string-concatenation of _first_ and _rest_.
1. Let _timeFormat_ be _styles_.[[TimeFormat]].[[&lt;_timeStyleField_&gt;]].
Copy link
Contributor

Choose a reason for hiding this comment

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

More string manipulation that we don't want in the spec.

Copy link
Contributor Author

@ben-allen ben-allen Apr 28, 2023

Choose a reason for hiding this comment

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

What's best practice here -- table iteration when possible, or just accept that this is a place where by necessity 402 style guidelines must diverge from 262?

1. Assert: _styleFields_ is a Record (see <emu-xref href="#sec-Intl.DisplayNames-internal-slots"></emu-xref>).
1. Set _displayNames_.[[Fields]] to _styleFields_.
1. Return _displayNames_.
</emu-alg>
<emu-table id="table-displaynames-types">
<emu-caption>Fields Corresponding to Type Names</emu-caption>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, there must be a better way.

spec/relativetimeformat.html Outdated Show resolved Hide resolved
ben-allen and others added 2 commits April 28, 2023 09:04
typo fix

Co-authored-by: Richard Gibson <[email protected]>
table-iteration correction

Co-authored-by: Richard Gibson <[email protected]>
@ben-allen
Copy link
Contributor Author

ben-allen commented Apr 28, 2023

Temperature check: would it be most appropriate to:

  1. Revert all changes referenced by brace-enclosed field names in patterns
  2. Deeper dive on format patterns -- which is something I'd like to do out of selfish "this refactor is getting me familiar with every corner of the spec" motivations, or just
  3. Accept that refactoring to adhere to 262 style is a low priority for a reason.

@ryzokuken ryzokuken added the editorial Involves an editorial fix label Jun 29, 2023
@ben-allen ben-allen marked this pull request as draft August 3, 2023 16:26
@ben-allen
Copy link
Contributor Author

closed; abandoned for now

@ben-allen ben-allen closed this Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Involves an editorial fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editorial: Start all fields and slots with an uppercase code point
4 participants