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

Allow append to existing object; allow user-provided read/write functions #29

Merged
merged 7 commits into from
Oct 19, 2019

Conversation

derrell
Copy link
Contributor

@derrell derrell commented Oct 3, 2019

The following changes are included here:

  1. New Method: append

A new append method has been added. Whereas the traditional calls such as

store.set('a', { b : c });
store.set('a', { d : e });

allows the second call to overwrite the entire value of 'a', yielding

{
  d : e
}

this sequence:

store.append('a', { b : c });
store.append('a', { d : e });

ratains prior values, and thus yields:

{
  b : c,
  d : e
}
  1. The means of reading (and parsing), and writing to files can now be
    overridden by the caller, in the options argument. This allows adding
    additional error handling, in the simple case, or reading and writing entirely
    different types of files in the more sophisticated case. An example is
    provided in examples/ini.js that shows the store reading and writing an INI
    file, while allowing it to be manipulated programmatically in the standard
    data-store fashion.

…ions

The following changes are included here:

1. New Method: append

A new `append` method has been added. Whereas the traditional calls such as
```
store.set('a', { b : c });
store.set('a', { d : e });
```
allows the second call to overwrite the entire value of 'a', yielding
```
{
  d : e
}
```

this sequence:
```
store.append('a', { b : c });
store.append('a', { d : e });
```
ratains prior values, and thus yields:
```
{
  b : c,
  d : e
}
```

2. The means of reading (and parsing), and writing to files can now be
overridden by the caller, in the `options` argument. This allows adding
additional error handling, in the simple case, or reading and writing entirely
different types of files in the more sophisticated case. An example is
provided in `examples/ini.js` that shows the store reading and writing an INI
file, while allowing it to be manipulated programmatically in the standard
data-store fashion.
Copy link
Collaborator

@doowb doowb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Depending on @jonschlinkert feedback, I like this idea. I'm requesting a few changes and have a few questions in this review. Also, please add tests for the new options.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/test.js Show resolved Hide resolved
@derrell derrell requested a review from doowb October 4, 2019 17:43
@derrell
Copy link
Contributor Author

derrell commented Oct 4, 2019

I don't know mocha well enough (or at all, other than by inspection of existing tests) to be able to add tests for the new options. That requires not doing the beforeEach section, where the store is automatically created, and instead doing one or more of those with the new options. I think I'll have to leave those tests to someone who knows this test infrastructure. The examples/ini.js shows it working and could possibly be used as a template for tests.

Otherwise, I think all questions have been answered and addressed. Hope you find this good to go.

Thanks.

@doowb
Copy link
Collaborator

doowb commented Oct 4, 2019

@derrell thanks for the making the requested changes. I'll look at this over the weekend and try it out using the example code.

examples/ini.js Outdated Show resolved Hide resolved
@derrell derrell requested a review from doowb October 5, 2019 00:34
@derrell
Copy link
Contributor Author

derrell commented Oct 9, 2019

A friendly ping...

@doowb
Copy link
Collaborator

doowb commented Oct 10, 2019

Sorry, I didn't get to this last weekend. I'll check it out this weekend.

Copy link
Collaborator

@doowb doowb left a comment

Choose a reason for hiding this comment

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

@derrell sorry for the delay in getting to this. All of the changes I'm requesting here are linting changes to keep with the same syntax style of the project. Most of them I was able to use the "suggest" feature on GitHub, but there are a couple of multi-line changes that I wasn't able to use that.

Thanks again, and I think this looks good after those nitpicks. I did pull this down to run the example and tests.

examples/ini.js Outdated Show resolved Hide resolved
examples/ini.js Outdated Show resolved Hide resolved
examples/ini.js Outdated Show resolved Hide resolved
examples/ini.js Outdated Show resolved Hide resolved
examples/ini.js Outdated Show resolved Hide resolved
examples/ini.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@derrell
Copy link
Contributor Author

derrell commented Oct 14, 2019

You got it! Yes, I always try to maintain style consistency with whatever code I'm editing, but habits are... well, they're habitual. :-)

All should be corrected now.

@derrell derrell requested a review from doowb October 14, 2019 11:10
@doowb
Copy link
Collaborator

doowb commented Oct 15, 2019

@derrell thanks, there are a still a few places with requested changes. (BTW... I made them granular because GitHub makes it easy to do single line "suggestions" and to resolve "outdated" conversations)

@derrell
Copy link
Contributor Author

derrell commented Oct 19, 2019

Sigh. Github covered those up with a "hidden conversations" that I didn't notice. I think all are now showing as outdated, so hopefully we're all set.

@doowb
Copy link
Collaborator

doowb commented Oct 19, 2019

Thanks! Sorry for all the nitpicks, I'm going to merge this in and I'll get the version bumped and published today or tomorrow morning.

@doowb doowb merged commit bd96ee9 into jonschlinkert:master Oct 19, 2019
@derrell
Copy link
Contributor Author

derrell commented Oct 19, 2019

Thanks!

@derrell
Copy link
Contributor Author

derrell commented Dec 2, 2019

Hi. I've been using the source version to date, so hadn't noticed... It seems that the new version never got published?

@derrell derrell mentioned this pull request Dec 8, 2019
@derrell
Copy link
Contributor Author

derrell commented Dec 14, 2019

@doowb Just a ping to see if you wanted to publish this latest version now that you've been given permission... Thanks.

@doowb
Copy link
Collaborator

doowb commented Dec 14, 2019

@derrell I had realized that the set-value library allows passing in a merge option, but data-store wasn't handling options in a way that could be passed through. After talking about this with Jon, I implemented #35. The changes also allow passing through a custom merge function, which you can see in this test.

I'll be merging that PR in soon and publishing it to npm. There were also a few naming changes with the new options that you introduced. Thanks for your original PR and sorry for the delay in getting it published.

@derrell
Copy link
Contributor Author

derrell commented Dec 15, 2019

Thanks. Eagerly awaiting. It looks like examples/ini.js will need an update to the new naming scheme as well; it uses the names I had added.

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.

None yet

2 participants