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

docs: add language-specific initialize naming #244

Conversation

maxveldink
Copy link
Member

This PR

Adds callout for language-specific initialize function naming. This is an idea @toddbaert had in this review.

Signed-off-by: Max VelDink <maxveldink@gmail.com>
@austindrenski
Copy link
Member

austindrenski commented Feb 8, 2024

This is great, and also lines up with discussions in the dotnet sdk to begin post-fixing spec-defined async method names with Async to match dotnet/BCL conventions.

Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

Nice thanks for the change!
In other cases we just used wording like The API MUST have a method for setting evaluation context for a domain..
I feel this could be a bit cleaner than using a name and then stating that it can be renamed.

Could be something like:

The provider MAY define an initialization function which accepts the global evaluation context as an argument and performs initialization logic relevant to the provider.
or:
The provider MAY define a method for initialization which accepts the global evaluation context as an argument and performs initialization logic relevant to the provider.
or:
The provider MAY define a mechanism for initialization which accepts the global evaluation context as an argument and performs initialization logic relevant to the provider.

I would also accept the change proposed by you but just wanted to propose this.
Not sure what others think?

@austindrenski
Copy link
Member

@lukas-reining sounds fine to me. The only question I'd have is if we should still have a suggested name mentioned with the text you quoted? (not sure if there are norms/patterns around this type of thing in other language-agnostic CNCF specs)

@lukas-reining
Copy link
Member

@austindrenski I think we do not have to suggest a name. This is as we do in the other cases🤔

@toddbaert
Copy link
Member

I think we can avoid a specific name.

Nit: but I recommend you flow the new sentence to the next line, which makes review easier. This doesn't generally impact markdown rendering (though there was a recent regression on openfeature.dev that now includes the source spacing 😅 ).

@toddbaert toddbaert self-requested a review February 8, 2024 15:55
maxveldink and others added 2 commits February 8, 2024 12:57
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Max VelDink <maxveldink@gmail.com>
@maxveldink
Copy link
Member Author

@toddbaert @lukas-reining Thanks for the suggestions; this is my first spec language PR, so I appreciate the guidance! I like the idea of avoiding the stricter RFC2119 terminology; the goal is to add a simple callout to aid SDK authors. Let me know if that language sounds good!

@lukas-reining
Copy link
Member

@maxveldink what do you think of changing the requirement 2.4.1 as stated here to
The provider MAY define an initialization function which accepts the global evaluation context as an argument and performs initialization logic relevant to the provider.

I think this is more consistent with the rest of the spec and we do not have to add the note that the name might be changed respecting idioms to every other function that is speced this way.
When talking about this function we can just call it initialization function.

@maxveldink
Copy link
Member Author

@lukas-reining I like it! I didn't realize you were referring to changing the actual requirement language. Let me make that update!

Signed-off-by: Max VelDink <maxveldink@gmail.com>
@maxveldink maxveldink force-pushed the maxveldink/add-language-call-out-for-init branch from 2cf4fb0 to 1a7cf98 Compare February 9, 2024 11:50
@maxveldink maxveldink enabled auto-merge (squash) February 9, 2024 11:51
@toddbaert
Copy link
Member

LGTM! @lukas-reining please merge if you agree.

Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

Great @maxveldink looks good to me!

@maxveldink maxveldink merged commit 2c36abd into open-feature:main Feb 9, 2024
6 checks passed
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.

5 participants