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: Don't use dot access in most places #90

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

a-sully
Copy link
Collaborator

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

@a-sully
Copy link
Collaborator Author

a-sully commented Jan 12, 2023

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
Copy link
Member

annevk commented Jan 12, 2023

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
Copy link
Collaborator Author

a-sully commented Jun 14, 2023

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?

index.bs Outdated Show resolved Hide resolved
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
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
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?

index.bs Outdated Show resolved Hide resolved
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
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
Member

Choose a reason for hiding this comment

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

Filed #132.

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
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
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
Copy link
Collaborator Author

a-sully commented Jun 14, 2023

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