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

docs(data-point/README.md): State in README that constants should not contain other reducers #213

Merged

Conversation

abepeterkin
Copy link
Collaborator

@abepeterkin abepeterkin commented Feb 13, 2018

closes #201

@coveralls
Copy link

coveralls commented Feb 13, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7c2450f on abepeterkin:docs/prevent-reducers-inside-of-constants into a636938 on ViacomInc:master.

@raingerber
Copy link
Contributor

Just fyi you should preface the "#201" with a keyword like "fix" or "closes":

https://help.github.com/articles/closing-issues-using-keywords/

raingerber
raingerber previously approved these changes Feb 14, 2018
paulmolluzzo
paulmolluzzo previously approved these changes Feb 14, 2018
Copy link
Collaborator

@paulmolluzzo paulmolluzzo left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@acatl acatl left a comment

Choose a reason for hiding this comment

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

can we change the wording a bit, i believe its not that yoy should not pass a reducer, but that which ever value you pass will be evaluated only as a constant/static value ? @raingerber @paulmolluzzo ?

@paulmolluzzo
Copy link
Collaborator

paulmolluzzo commented Feb 15, 2018

@acatl I see your point, sure. 🤔 Maybe it's good to keep this small line and add what you wrote too? Something like:

The constant reducer always returns the given value, and any value passed to constant will be evaluated only as a constant/static value. Constants that contain other reducers will not be evaluated, but will be returned as a function.

I'm not sure of the wording of the last sentence, tbh. It also feels like we're repeating the same thing a few times. (e.g. "Constants are constant and return a static/constant value...") 😅

  • Edited like 25x to get the formatting right. 🙈

@raingerber
Copy link
Contributor

raingerber commented Feb 15, 2018

I can't think of a better way to explain this in words, but having an example to go with the current text might help. Here's a quick example that (hopefully) could be shortened:

const DataPoint = require('data-point')

const { constant } = DataPoint.helpers

const dataPoint = DataPoint.create()

const input = {
  b: 1
}

// ReducerObject that contains a ReducerPath ('$a')
let reducer = {
  a: '$b'
}

dataPoint.resolve(reducer, input) // => { a: 1 }

// both the object and the path will be treated as
// constants instead of being used to create reducers
reducer = constant({
  a: '$b'
})

dataPoint.resolve(reducer, input) // => { a: '$b' }

@acatl
Copy link
Collaborator

acatl commented Feb 21, 2018

closing this PR since it hasn't been active, please re-open it once you have it ready

@acatl acatl closed this Feb 21, 2018
@abepeterkin
Copy link
Collaborator Author

Instead of changing the description of constants to say you shouldn't put reducers inside of constants, I added the example @raingerber made in order to show what happens when you do.

@@ -1211,6 +1211,34 @@ constant(value:*):*
```
</details>
Copy link
Contributor

Choose a reason for hiding this comment

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

we should still keep the "Constants should not contain other reducers." sentence that you originally added

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea

Copy link
Collaborator

Choose a reason for hiding this comment

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

I rather have it taken off or changed since the actual content of a constant could be later on used to build a reducer. The only fact is that constant will not evaluate their content, they can contain any type of value. So this comment is misleading.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if we pass a reducer function as a constant, on a second iteration another reducer could use this value to execute itself

'model:foo': {
   value: [
      { a: c((val) => val * 2) },
      (val) => val.a(2)
   ]
}

This only applies to Function Reducers, what I'm trying to articulate is just that constants can have anything, even a reducer if they want, just nothing will be evaluated, everything is taken as-is

@@ -1211,6 +1211,34 @@ constant(value:*):*
```
</details>


<details>
<summary>constants that contain other reducers will not be evaluated</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

this summary is a little misleading, since these constants are still "evaluated". not sure how to reword it though; maybe something like this?

reducers are not evaluated when they're defined inside of constants

Copy link
Collaborator

Choose a reason for hiding this comment

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

More specifically, no functions inside of a constant are evaluated, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true, but this particular example doesn't have a function inside of a constant. If we do add that to the example (which wouldn't hurt), we still want to get across the fact that any reducer type inside a constant is not evaluated

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, thanks for confirming.

@raingerber raingerber changed the title docs(data-point/README.md): Specified in the README that constant red… docs(data-point/README.md): State in README that constants should not contain other reducers Feb 23, 2018
Re-added the line about constants not containing other reducers, changed the summary of the new
example

ViacomInc#213
raingerber
raingerber previously approved these changes Feb 23, 2018
paulmolluzzo
paulmolluzzo previously approved these changes Feb 23, 2018
Copy link
Collaborator

@acatl acatl left a comment

Choose a reason for hiding this comment

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

Please remove the section 'Constants should not contain other reducers.' everything else looks good to me.

@@ -1211,6 +1211,34 @@ constant(value:*):*
```
</details>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I rather have it taken off or changed since the actual content of a constant could be later on used to build a reducer. The only fact is that constant will not evaluate their content, they can contain any type of value. So this comment is misleading.

@raingerber
Copy link
Contributor

the actual content of a constant could be later on used to build a reducer

It doesn't really make sense to do that, and I don't think it's actually possible.

@acatl acatl dismissed stale reviews from paulmolluzzo and raingerber via 485e940 February 25, 2018 11:52
acatl
acatl previously approved these changes Feb 25, 2018
paulmolluzzo
paulmolluzzo previously approved these changes Feb 25, 2018
Copy link
Collaborator

@paulmolluzzo paulmolluzzo left a comment

Choose a reason for hiding this comment

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

Still looks good to me. As long as everyone’s OK with the phrasing.




in options](#options-with-constants) example.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra lines?

@acatl acatl dismissed stale reviews from paulmolluzzo and themself via da9a1c0 February 26, 2018 14:28
@acatl acatl merged commit 5e7588f into ViacomInc:master Feb 26, 2018
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

Successfully merging this pull request may close these issues.

Prevent reducers inside of constants
5 participants