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

Rename selector to partialfilterselector in indexes #818

Merged
merged 2 commits into from Sep 21, 2017
Merged

Rename selector to partialfilterselector in indexes #818

merged 2 commits into from Sep 21, 2017

Conversation

garrensmith
Copy link
Member

To make it easier to distinguish between a selector in _find and a
selector in _index. Rename the selector in the _index to
partialfilterselector. It also gives a bit more of an explanation of
what this selector does.

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

@willholley
Copy link
Member

It needs rebasing on master but basically looks good to me. What happens if both selector and partialfilterselector are specified?

@garrensmith
Copy link
Member Author

@willholley I've just rebased

@willholley
Copy link
Member

@garrensmith I think we should follow the convention for other fields and use _ as a delimiter - i.e. partial_filter_selector instead of partialfilterselector.

Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

partial_filter_selector for a name would be better to match the established naming convention.

Copy link
Member

@willholley willholley left a comment

Choose a reason for hiding this comment

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

Nearly there, I think. Possibly an issue with existing indexes that have a selector property though.

@@ -92,7 +92,7 @@ maybe_filter_indexes_by_ddoc(Indexes, Opts) ->
{use_index, []} ->
%We remove any indexes that have a selector
Copy link
Member

Choose a reason for hiding this comment

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

probably add a space here

@@ -44,7 +44,7 @@
to_json/1,
delete/4,
get_usable_indexes/3,
get_idx_selector/1
get_idx_partial_filter_selector/1
Copy link
Member

Choose a reason for hiding this comment

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

rename to get_partial_filter_selector - that it is in the mango_idx module implies that it relates to an index

Mod:validate_new(Idx1, Db).


maybe_rename_idx_fields({Props}) ->
Copy link
Member

Choose a reason for hiding this comment

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

rename to maybe_rename_fields

@@ -185,7 +185,7 @@ def test_lt_includes_null_but_not_missing(self):

def test_lte_includes_null_but_not_missing(self):
docs = self.db.find({
"twitter": {"$lt": 1}
"twitter": {"$lte": 1}
Copy link
Member

Choose a reason for hiding this comment

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

move this into a separate PR as it's unrelated

@willholley
Copy link
Member

after squashing, looks ok to me

@garrensmith
Copy link
Member Author

@davisp I've made all the required changes. Could you take a look when you get a chance.

@@ -199,8 +199,8 @@ opts() ->
{tag, fields},
{validator, fun mango_opts:validate_sort/1}
]},
{<<"selector">>, [
{tag, selector},
Copy link
Member

Choose a reason for hiding this comment

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

I see the get_legacy_selector code above, but reading through code I can't convince myself that removing this option definition won't break old indexes that used the selector name. I wonder if we shouldn't continue to support this for awhile and upgrade it internally for new indexes?

Copy link
Member

Choose a reason for hiding this comment

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

And/or perhaps just adding a test specifically for the upgrade would be enough here. And for that I'd like to see us write an old style ddoc directly and then read the index to make sure it behaves as expected.

To make it easier to distinguish between a selector in _find and a
selector in _index. Rename the selector in the _index to
partialfilterselector. It also gives a bit more of an explanation of
what this selector does.
Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1

@garrensmith garrensmith merged commit 00df0de into apache:master Sep 21, 2017
wohali pushed a commit that referenced this pull request Oct 19, 2017
To make it easier to distinguish between a selector in _find and a
selector in _index. Rename the selector in the _index to
partialfilterselector. It also gives a bit more of an explanation of
what this selector does.
willholley pushed a commit to willholley/couchdb that referenced this pull request May 22, 2018
To make it easier to distinguish between a selector in _find and a
selector in _index. Rename the selector in the _index to
partialfilterselector. It also gives a bit more of an explanation of
what this selector does.
@garrensmith garrensmith deleted the rename-to-partialfilterselector branch December 12, 2019 17:19
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

3 participants