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.

Sign up for GitHub

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

Jump to bottom

options.ignore #29

Merged
merged 9 commits into from
May 2, 2016
Merged

options.ignore #29

merged 9 commits into from
May 2, 2016

Conversation

gr2m
Copy link
Collaborator

@gr2m gr2m commented Apr 20, 2016

follow up for #25, based on #26

@gr2m gr2m mentioned this pull request Apr 20, 2016
Closed
@gr2m gr2m force-pushed the issue/13/based-on-v2a branch 2 times, most recently from c1cb457 to 201407e Compare April 20, 2016 00:27
@gr2m gr2m mentioned this pull request Apr 20, 2016
Closed
@xMartin
Copy link

xMartin commented Apr 22, 2016

Works for me.

As the option is called "ignore" we should probably change the title of this issue to avoid confusion.

As I understand it, the {ignore: '_attachments'} option is currently required if you're not 100% sure there will never be attachments in your documents. With this option set, db.putAttachment / db.getAttachment are working for me, so the note in the README is misleading.

@gr2m gr2m changed the title options.exclude options.ignore Apr 22, 2016
@gr2m gr2m force-pushed the issue/13/based-on-v2a branch from 201407e to 0572dbe Compare April 22, 2016 13:14
@calvinmetcalf
Copy link
Owner

I don't think this was built on master

@gr2m gr2m force-pushed the issue/13/based-on-v2a branch from 0572dbe to 696a1af Compare April 27, 2016 13:48
calvinmetcalf
calvinmetcalf reviewed Apr 27, 2016
View reviewed changes
index.js
var db = this;
var key, pub;
var turnedOff = false;
return db.get(configId).catch(function (err) {
var ignore = ['_id', '_rev']
var modP
Copy link
Owner

Choose a reason for hiding this comment

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

we don't need modp any more

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops sorry I had it removed, the rebase must have readded it, I’ll fix in in a minute

@gr2m gr2m force-pushed the issue/13/based-on-v2a branch from 696a1af to 9014984 Compare April 27, 2016 18:12
@gr2m
Copy link
Collaborator Author

gr2m commented Apr 27, 2016

done updating

@calvinmetcalf
Copy link
Owner

I just got back from vacation and am digging out so I'll try to get to this sometime today or tommorow

calvinmetcalf
calvinmetcalf reviewed May 1, 2016
View reviewed changes
index.js
return db.get(configId).catch(function (err) {
var ignore = ['_id', '_rev']

if (options && options.ignore) {
Copy link
Owner

Choose a reason for hiding this comment

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

we probably want to check it's actually an array

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no need, .concat takes care of it

> [1].concat(2)
[ 1, 2 ]
> [1].concat([3,4])
[ 1, 3, 4 ]

@calvinmetcalf
Copy link
Owner

few nits otherwise looks good

@calvinmetcalf
Copy link
Owner

looking good, feel free to squash and merge

@gr2m gr2m merged commit 84a49a1 into master May 2, 2016
@gr2m gr2m deleted the issue/13/based-on-v2a branch May 2, 2016 18:18
@gr2m
Copy link
Collaborator Author

gr2m commented May 2, 2016

I’ve added and pushed a build commit, too (7feae75). I think ít’s ready for a new release

@gr2m gr2m mentioned this pull request May 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gr2m @xMartin @calvinmetcalf