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

Adding sortable tag field support & UNF support #1862

Merged
merged 9 commits into from
Oct 1, 2021
Merged

Adding sortable tag field support & UNF support #1862

merged 9 commits into from
Oct 1, 2021

Conversation

slorello89
Copy link
Collaborator

Adding the ability to add sorted tag fields in indices per #1838

Note: When serializing the arguments RediSearch cares about the order of the separator and sortable arguments, hence after calling base.SerializeRedisArgs, if the index is sortable and removes the sortable flag, and then adds it back after the separator flag is added.

@dcoopertd
Copy link

While you are in there, could support for the new UNF be added as well? RediSearch/RediSearch#2188

@AvitalFineRedis
Copy link
Contributor

@dcoopertd https://github.com/slorello89/StackExchange.Redis/pull/1

@slorello89 slorello89 changed the title Adding sortable tag field support Adding sortable tag field support & UNF support Sep 19, 2021
@mgravell
Copy link
Collaborator

mgravell commented Oct 1, 2021

The main problem I can see here is the breaking change on TextField's ctor and the AddSortableTextField method which is going to cause MissingMethodException on anything that takes a transient dependency (i.e. not rebuilt explicitly). We usually try hard to avoid hard breaks, with the preferred method being: to add overloads rather than optional args.

For example:

- public TextField(string name, double weight = 1.0, bool sortable = false, bool noStem = false, bool noIndex = false)
- : base(name, FieldType.FullText, sortable, noIndex)
+ public TextField(string name, double weight = 1.0, bool sortable = false, bool noStem = false, bool noIndex = false, bool unf = false)
+ : base(name, FieldType.FullText, sortable, noIndex, unf)

could be achieved instead via:

public TextField(string name, double weight, bool sortable, bool noStem, bool noIndex)
  : this(name, weight, sortable, noStem, noIndex, false) {}
public TextField(string name, double weight = 1.0, bool sortable = false, bool noStem = false, bool noIndex = false, bool unf = false)
  • by adding the overload, we don't break existing code (noting that adding/removing default parameter values does not cause a MissingMethodException)
  • by removing the parameter defaults, we ensure that most rebuilt new code goes to the new method, and doesn't pay any indirection overhead

Likewise:

- public Schema AddSortableTextField(string name, double weight = 1.0)
+ public Schema AddSortableTextField(string name, double weight = 1.0, bool unf = false)

could be

public Schema AddSortableTextField(string name, double weight)
  => AddSortableTextField(name, weight, false);
public Schema AddSortableTextField(string name, double weight = 1.0, bool unf = false)

@mgravell
Copy link
Collaborator

mgravell commented Oct 1, 2021

If we can fix ^^^, and add something to releasenotes.md, we should be set?

@mgravell
Copy link
Collaborator

mgravell commented Oct 1, 2021

oh: additional thought: what is unf? the fact that I need to ask suggests that it needs to be clearer; perhaps unNormalizedForm would be clearer than unf?

I also wonder whether we're getting into the territory where a [Flags] enum might be a better choice than a myriad of bool values?

Copy link
Collaborator

@mgravell mgravell left a comment

Choose a reason for hiding this comment

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

Lgtm, ta

@mgravell mgravell merged commit 8775036 into StackExchange:main Oct 1, 2021
@slorello89
Copy link
Collaborator Author

The main problem I can see here is the breaking change on TextField's ctor and the AddSortableTextField method which is going to cause MissingMethodException on anything that takes a transient dependency (i.e. not rebuilt explicitly). We usually try hard to avoid hard breaks, with the preferred method being: to add overloads rather than optional args.

TIL -> thanks for pointing that out.

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.

4 participants