Skip to content

Restore name of second argument to FileReader.readAsText #105

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

Closed
blixt opened this issue Jul 21, 2018 · 9 comments
Closed

Restore name of second argument to FileReader.readAsText #105

blixt opened this issue Jul 21, 2018 · 9 comments

Comments

@blixt
Copy link
Contributor

blixt commented Jul 21, 2018

In eaf8fd9 ("Cleaned up wrong TOC"), arangana made a change from readAsText(blob[, encoding]) to readAsText(blob[, label]) but I have not been able to find a motivation.

I'm proposing that the name encoding is brought back in order to clarify what the parameter does, without having to read the surrounding text in the spec.

My main motivation for filing this issue is that some documentation generators depend on the wording in this spec, which trickles down an arguably poor parameter name choice to all developers using the DOM APIs. (See microsoft/TypeScript-DOM-lib-generator#536).

The reasoning for encoding specifically is:

  1. It's the parameter name that had already been settled on in the spec from 2009–2013
  2. It's still used by respected documentation sources (eg., MDN)
  3. It's arguably the most concise description of what the valid input values are (eg., "UTF-8")

Here is what today's documentation looks like in Visual Studio Code (TypeScript) due to the name choice in this repository:

screen shot 2018-07-21 at 12 55 53

@inexorabletash
Copy link
Member

encodingLabel might be even better, but given that we don't reify encodings as objects (yet?) it is probably overkill.

So +1 for encoding from me.

@blixt
Copy link
Contributor Author

blixt commented Jul 23, 2018

@inexorabletash This is my first "contribution" to a W3C project so I'm not sure how you prefer to do this. Can I just submit a PR to change the parameter name and we let its eventual merge/close serve as the vote?

@chaals
Copy link

chaals commented Jul 23, 2018

That should work fine.

This change technically changes conformance and is therefore substantive, but if the only thing that actually changes is a label then it's fine from an IPR perspective and we don't need any more commitment. (If you propose some substantive thing we ask that you make a licensing commitment not to put patented stuff in the spec, unless you grant a royalty-free license to it...)

@annevk
Copy link
Member

annevk commented Jul 23, 2018

It's not substantive to change the name of a parameter. It doesn't affect conformance or implementations.

@blixt
Copy link
Contributor Author

blixt commented Jul 23, 2018

Thanks. And sorry for the newbie question but I couldn't find this in CONTRIBUTING.md – should I only edit index.bs? Is there a generator tool I should run to update timestamps/references?

I've got the change ready to push for all three files (index.bs, TR.html, index.html) but I'm not sure whether the intent is to leave some of those files alone.

@inexorabletash
Copy link
Member

It's probably easiest to just touch index.bs and let one of the editors regenerate the .html using the bikeshed tool. TR.html is a snapshot and should be left alone.

Maybe we can rope @rtoy into setting up automatic bikeshedding for this spec too now that he's an expert?

@rtoy
Copy link

rtoy commented Jul 23, 2018

Sure, I can do this. But not for a week or two. Or you can do this for yourself by copying .travis.yml, compile.sh and deploy.sh from https://github.com/wicg/cookie-store. I think the only thing needed is to use your admin powers and create a new deploy key (with write access otherwise it won't work), update .travis.yml appropriately.

@siusin
Copy link
Contributor

siusin commented Aug 1, 2018

Auto-deploy workflow set by f32361a.

@blixt @saschanaz could you rebase your pull request to the master branch please? We've removed index.bs from gh-pages.

@blixt
Copy link
Contributor Author

blixt commented Aug 3, 2018

@siusin I've rebased and changed the target branch of the PR to master since the file I changed no longer exists in gh-pages.

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

No branches or pull requests

6 participants