Skip to content

Editorial: Don't use dot access in most places#90

Merged
annevk merged 2 commits into
whatwg:mainfrom
a-sully:dont-use-dot-access-for-concepts
Jun 15, 2023
Merged

Editorial: Don't use dot access in most places#90
annevk merged 2 commits into
whatwg:mainfrom
a-sully:dont-use-dot-access-for-concepts

Conversation

@a-sully

@a-sully a-sully commented Jan 12, 2023

Copy link
Copy Markdown
Collaborator

@a-sully

a-sully commented Jan 12, 2023

Copy link
Copy Markdown
Collaborator Author

I believe none of the other instances of dot access are for "concepts" but please correct me if I'm wrong

@a-sully a-sully requested a review from annevk January 12, 2023 17:58
@annevk

annevk commented Jan 12, 2023

Copy link
Copy Markdown
Member

So accessing dictionary members through dots is also wrong. Dictionaries are to be treated as maps.

I think the only correct dot usage is in serialization and deserialization.

@a-sully a-sully force-pushed the dont-use-dot-access-for-concepts branch from 7aa5fda to c541310 Compare June 14, 2023 05:06
@a-sully a-sully changed the title Editorial: Don't use dot access on lock release Editorial: Don't use dot access in most places Jun 14, 2023
@a-sully

a-sully commented Jun 14, 2023

Copy link
Copy Markdown
Collaborator Author

Rebased this on the infra changes introduced in #95 (and updated to remove use of dot access in most places, as you suggest in your last comment). PTAL?

Comment thread index.bs Outdated
Comment thread index.bs Outdated
1. Let |command| be |input|.{{WriteParams/type}} if |input| is a {{WriteParams}},
and {{WriteCommandType/"write"}} otherwise.
1. Let |command| be |input|'s {{WriteParams/type}} if
|input| is a {{WriteParams}}; otherwise {{WriteCommandType/"write"}}.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is a [=/dictionary=]*

We don't know the dictionary type at this point. Existing issue though so feel free to file a follow-up. (Or I could.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sounds like we can't check the specific type (i.e. WriteParams) of a union? Just the data structure (i.e. dictionary/boolean/etc)

Would you mind filing a follow-up?

Comment thread index.bs Outdated
Comment thread index.bs
Comment on lines +1211 to +1215
1. If |input| is `undefined` or |input| is a [=/dictionary=] and
|input|["{{WriteParams/data}}"] does not [=map/exists|exist=],
[=/reject=] |p| with a {{TypeError}} and abort these steps.
1. Let |writePosition| be |stream|.[=[[seekOffset]]=].
1. If |input| is a {{WriteParams}} and |input|.{{WriteParams/position}} is not `undefined`,
set |writePosition| to |input|.{{WriteParams/position}}.
1. Let |oldSize| be |stream|.[=[[buffer]]=]'s [=byte sequence/length=].
1. Let |data| be |input|["{{WriteParams/data}}"] if
|input| is a [=/dictionary=]; otherwise |input|.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

FYI: this is a no-op, but which allows us to check [=map/exists=] rather than rely on the dictionary access returning undefined

This reveals a separate issue that |input| comes from "converting to an IDL value" algorithm, which appears to be able to return both null and undefined? Is that a problem?

I've left it as-is for now so that this CL can remain "Editorial" but happy to address this in a follow-up if it's a problem

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Filed #132.

Comment thread index.bs
1. [=/Resolve=] |p|.
1. Otherwise, if |command| is {{WriteCommandType/"seek"}}:
1. If |chunk|.{{WriteParams/position}} is `undefined`,
1. [=Assert=]: |chunk| is a [=/dictionary=].

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

not sure whether this counts as a non-editorial change... the previous text assumes this to be true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Removing one might not be, but adding one seems fine.

@a-sully

a-sully commented Jun 14, 2023

Copy link
Copy Markdown
Collaborator Author

Feel free to merge if this looks good. Thanks!

@annevk annevk merged commit d636c34 into whatwg:main Jun 15, 2023
@a-sully a-sully deleted the dont-use-dot-access-for-concepts branch June 15, 2023 19:58
a-sully added a commit to a-sully/file-system-access that referenced this pull request Jun 20, 2023
Follow-up to whatwg/fs#90

Also fixes a use of FileSystemPermissionDescriptor in
the showDirectoryPicker() method algorithm
a-sully added a commit to a-sully/file-system-access that referenced this pull request Jun 22, 2023
Follow-up to whatwg/fs#90

Also fixes a use of FileSystemPermissionDescriptor in
the showDirectoryPicker() method algorithm
a-sully added a commit to WICG/file-system-access that referenced this pull request Jun 22, 2023
Follow-up to whatwg/fs#90

Also fixes a use of FileSystemPermissionDescriptor in
the showDirectoryPicker() method algorithm
github-actions Bot added a commit to WICG/file-system-access that referenced this pull request Jun 22, 2023
Follow-up to whatwg/fs#90

Also fixes a use of FileSystemPermissionDescriptor in
the showDirectoryPicker() method algorithm

SHA: 739b0c4
Reason: push, by @a-sully

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants