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

Add temp var to real-world examples #254

Closed
wants to merge 1 commit into from

Conversation

vjpr
Copy link

@vjpr vjpr commented Oct 22, 2021

Added temp vars to real-world examples.

Related: #253

I I think it might be useful because for some examples it demonstrates the difficulty in finding an appropriate name (as mentioned in other issues), notably the functional programming examples. But then for other examples, it can be seen that things become less clear.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2021

Which examples do you find are less clear with pipes than with temp vars?

@vjpr
Copy link
Author

vjpr commented Oct 22, 2021

unpublish.js (temp vars = better)

const firstPkg = pkgs[0]
const {escapedName} = npa(firstPkg)
const json = await npmFetch.json(escapedName, opts); // Maybe escapedNameOfFirstPkg is better.

const json = pkgs[0] |> npa(%).escapedName |> await npmFetch.json(%, opts);

I can look at the last line and see what is being fetched without having to trace back through the pipeline.

underscore.js / ranmda (pipes = better)

Naming temporary functions becomes tedious here.

jest-cli.js (temp vars = win)

// Temp vars
const envvars = Object.keys(envars)
  .map(envar => `${envar}=${envars[envar]}`)
  .join(' ')}
const str = `$ ${envvars}`
const val = chalk.dim(str,  'node', args.join(' '))
console.log(val);

// With pipes
Object.keys(envars)
  .map(envar => `${envar}=${envars[envar]}`)
  .join(' ')
  |> `$ ${%}`
  |> chalk.dim(%, 'node', args.join(' '))
  |> console.log(%);

You have to do a double-take to figure out what this is. |>$ ${%}`. It's much clearer if there is a descriptive temp var being used.

unit.js - (temp vars = win)

// Temp vars
const doc = context && context.nodeType ? context.ownerDocument || context : document
const parsedHtml = jQuery.parseHTML(match[1], doc, true)
const mergedHtml = jQuery.merge(parsedHtml);
// With pipes
context
  |> (% && %.nodeType ? %.ownerDocument || % : document)
  |> jQuery.parseHTML(match[1], %, true)
  |> jQuery.merge(%);

@noppa
Copy link
Contributor

noppa commented Oct 22, 2021

Does the use of parsedHtml variable really make things more clear? I mean, isn't it obvious that the result of parseHTML is parsed html?

@mAAdhaTTah
Copy link
Collaborator

Besides maybe the first example, I disagree that any of the temp var versions are better. But more importantly, the README is intended to make the case for the operator, not just a collection of random comparisons. If there are examples that we don't think are improvements, we should probably remove them and replace them with better examples.

@vjpr
Copy link
Author

vjpr commented Oct 22, 2021

@noppa Does the use of parsedHtml variable really make things more clear? I mean, isn't it obvious that the result of parseHTML is parsed html?

I didn't actually know what parseHTML did.

jQuery.parseHTML uses native methods to convert the string to a set of DOM nodes

After reading this a better name is domNodes or nodes.

const doc = context && context.nodeType ? context.ownerDocument || context : document
const domNodes = jQuery.parseHTML(match[1], doc, true)
const mergedHtml = jQuery.merge(domNodes);

IDE features like IntelliJ/WebStorm/VSCode's inlay hints may negate the need for named temp vars, but depending on IDE is not a good IDE. Lots of code is reviewed outside an IDE.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2021

It's literally named "jquery parse html" - i can't conceive of having any expectation of what jquery will return after parsing html besides a jquery wrapper object around DOM nodes. Can you help me understand why anyone with any jQuery experience will have a different expectation, and what that might be?

@js-choi js-choi added the documentation Improvements or additions to documentation label Nov 18, 2021
@js-choi
Copy link
Collaborator

js-choi commented Nov 21, 2021

Thanks for your work here.

I’m going to go ahead and close this, since I don’t think any of the champions think comparing pipe code with both original code and temp-variable code is appropriate in the real-world examples section. I’m sorry if that’s disappointing; hopefully this is understandable.

After all, the point of the real-world examples section is to compare this proposal with status-quo code. Adding a third version to each example would just be confusing. (We could arguably even omit the original versions from each example, since the primary purpose is to show the pipe operator in action.)

In addition, there’s already a comparison with temp-var approaches in the “Why a pipe operator” section. A big point that it makes is that the examples’ original writers could have written their code with more temp variables but already chose not to, instead opting for deeply nested expressions, presumably for brevity. See also #211.

We appreciate the time you took in helping out! Thank you again for your work.

@js-choi js-choi closed this Nov 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants