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

Implement Setting class #102

Merged
merged 19 commits into from
Dec 24, 2023
Merged

Implement Setting class #102

merged 19 commits into from
Dec 24, 2023

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Dec 18, 2023

Description

Implements the Setting class.

Context:

  • Settings class already committed is a key-value map with mostly string values (or nested maps)
  • This Setting class is an individual setting which will be stored in that Settings map with a current (string) value
  • When fetching the (string) settings they are parsed using the Parser functional interface (callable in Python) to a typed value

Issues Resolved

Part of #84

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (62eac88) 100.00% compared to head (4ec936a) 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##              main      #102    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           61        78    +17     
  Lines         1627      2058   +431     
==========================================
+ Hits          1627      2058   +431     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Is syntax for callables ok?
Did I properly implement the IntegerParser?

Yes, but you can also object orient this, including subclassing a simpler Setting into ReadWritableSetting, and creating a SettingsParser#parse passed around as objects.

Better idea than the class method for the Java static intSetting() equivalent?

Should be a IntegerSetting() constructor that does all validation and parsing within.

src/opensearch_sdk_py/settings/setting.py Outdated Show resolved Hide resolved
src/opensearch_sdk_py/settings/setting.py Outdated Show resolved Hide resolved
@dbwiddis
Copy link
Member Author

you can also object orient this, including subclassing a simpler Setting into ReadWritableSetting

Hmm. On Java SDK we only use Setting in the code, the only time WriteableSetting comes into play is in the transport action which the user shouldn't have to deal with. So not sure subclassing is correct but I'll look into separating the transport protocol bits from the plain settings bits.

and creating a SettingsParser#parse passed around as objects.

Thought of this: in Java Parser is a functional interface with a single apply() method. The problem is that the different implementations have different transport (writeable) comms so we still need the equivalent of all these subclasses, or a big confusing class with all the individual methods inside it.

Should be a IntegerSetting() constructor that does all validation and parsing within.

I like this idea.

One other thing to note: the original port tried to faithfully model the exact Java classes, but I think making the default_value a callable is overcomplicating it from an extension standpoint: it's converted to its base type for transport anyway. I'll probably simplify that.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This looks good to me!

I still don't see what the advantage is of making a parser a callable instead of inheriting them all from an abstract class Settings > class Parser that implements a parse method?

@dbwiddis
Copy link
Member Author

This looks good to me!

Still in progress, need to handle TimeValue, ByteSizeValue (similar to the existing), Boolean (simpler), and String (which will involve validator) and then handle the transport actions to communicate them.

I still don't see what the advantage is of making a parser a callable instead of inheriting them all from an abstract class Settings > class Parser that implements a parse method?

I haven't fully decided not to switch yet, but am still leaning toward keeping it as is for now, as it matches the Java-side implementation so porting from Java SDK code is likely more straightforward. I think we can always switch later; isn't it essentially just replacing __call__ with parse and adding a new interface/abstract parent? It may make the transport easier at the end with an Optional[Parser].

It's a similar question with the validator and default value that I haven't fully decided on either and would probably want to implement similarly for all of them. Validator is much more obviously something we'd do a similar parent class for, but right now is only implemented for Strings. Default value is interesting as it takes a Settings argument but in the default (pun intended) case completely ignores it; the lambda notation makes that super-easy to do but leaves the door open for implementing a callable that reads another setting and chooses a default based on that... although we're stuck in the situation where we have to communicate that over transport and we may end up just wanting the return type there for simplicity.

TLDR: leaving as is for now but thinking it's an easy switch later. This whole setting system is more complex than you'd think and I'm handling it bite by bite (byte by byte?).

@dbwiddis
Copy link
Member Author

image

@dblock
Copy link
Member

dblock commented Dec 21, 2023

isn't it essentially just replacing __call__ with parse and adding a new interface/abstract parent? It may make the tra

Yes. I don't feel strongly about it. I like OO over function calls. Similarly you wouldn't build a SettingsProviderManagerFactoryControllerExecutor just because it exists in Java, but I get your point ;) I would lean on parse and validate and maybe comment about it.

LOL the XKCD is a great reminder that programming is just copy paste from SO (or another Java codebase)

@dbwiddis
Copy link
Member Author

copy paste from SO

Can't do that in an Apache project ;)

I will admit that the TimeValue class __str__ elegance was inspired by a certain LLM. I improved on it, of course.

@dbwiddis
Copy link
Member Author

dbwiddis commented Dec 22, 2023

@dblock
Copy link
Member

dblock commented Dec 22, 2023

Related to the XKCD, this is still one of the favorite pair of comments I've left in code: https://github.com/oshi/oshi/blob/35d3bd754fafdb82c72bcb0976809d8700b60aff/oshi-core/src/main/java/oshi/hardware/platform/windows/WindowsHWDiskStore.java#L122-L132

I couldn't get past the fact that PHYSICALDRIVE_PREFIX was not spelled PHYSICAL_DRIVE_PREFIX.

@dbwiddis
Copy link
Member Author

I couldn't get past the fact that PHYSICALDRIVE_PREFIX was not spelled PHYSICAL_DRIVE_PREFIX.

Blame Microsoft.

String PHYSICALDRIVE_PREFIX = "\\\\.\\PHYSICALDRIVE";

@dblock
Copy link
Member

dblock commented Dec 22, 2023

image

@dbwiddis
Copy link
Member Author

is just copy paste from SO

True story. I'm starting on the boolean parser. I dutifully hit Google with the search term "python str to bool" and got this as the top result:
Screenshot 2023-12-22 at 4 31 57 PM

Promising, eh? But clicking on the actual link leads to this:
Screenshot 2023-12-22 at 4 32 35 PM

Also, a boolean Christmas XKCD:
image

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis marked this pull request as ready for review December 23, 2023 04:48
@dbwiddis
Copy link
Member Author

Amazing how one class can turn into 32 files and 1402 lines of code...

Ready for review (you might just want to look at the last 2 commits).

@dbwiddis
Copy link
Member Author

image

Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis merged commit 535f6bb into opensearch-project:main Dec 24, 2023
7 checks passed
@dbwiddis dbwiddis deleted the setting branch December 24, 2023 03:37
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