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

Mark embedding options as advanced. #15081

Merged
merged 1 commit into from
May 7, 2024
Merged

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented May 7, 2024

No description provided.

cameel
cameel previously requested changes May 7, 2024
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Just doing a blank change request to prevent it from getting merged already, because I also had some suggestions for this from our previous discussion, but I have to run so I'll post specifics a bit later.

jasonp2203

This comment was marked as off-topic.

@ekpyron ekpyron dismissed cameel’s stale review May 7, 2024 18:45

Actually, let me be radical and aggressively make the point of: one thing at a time and dismiss the review - these are definitely advanced option, anything beyond that we can do in a new PR :-).

@ekpyron ekpyron merged commit 38ea398 into develop May 7, 2024
73 checks passed
@ekpyron ekpyron deleted the make-embedding-options-advanced branch May 7, 2024 18:45
@ekpyron
Copy link
Member

ekpyron commented May 7, 2024

(And even if we want do deal with this in a completely different manner, we can still do this in a new PR, i.e. this PR is better than develop alone, so we merge it and anything beyond that we can do afterwards)

@cameel
Copy link
Member

cameel commented May 8, 2024

Here's the relevant bit from that previous discussion:

I mean, rather than docs just better names would be nice. For example. STRICT_JSONCPP_VERSION sounds like it works like STRICT_Z3_VERSION. But instead it disables the version check completely. Why not something like ANY_JSONCPP_VERSION or ENABLE_JSONCPP_VERSION_CHECK?

Or USE_SYSTEM_LIBRARIES is very broad and I'd expect it to cover all libs like Boost, Z3 etc. This name needs some qualifier. The description of the option does not clarify it either.

Overall, this does not need to be extensively documented since I assume it's for development only, but at least names should be less ambiguous so that if you guess what it does, you have a chance to guess right :)

So basically, these options should just have a better names and descriptions.

The whole discussion on Matrix was longer, and there were other things mentioned, including whether we need this feature in the first place, but ultimately if we do have it, that's the one thing that should be improved IMO.

@cameel
Copy link
Member

cameel commented May 8, 2024

Also, a link to the original PR: #14860. I wanted to find it and expected that I would be somewhere in here already but apparently not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants