Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat #879: Different embedding styles prep #1273
Feat #879: Different embedding styles prep #1273
Changes from 5 commits
4e2f07a
e05c1c2
33e4438
e3fe400
3b1c903
7d97e78
2598689
35536c5
c1eaded
f86dc7e
7caac91
17baa5c
f4e8575
2f5fe5e
4f773f9
24a6b59
4190c43
090d941
7b9c95d
7365f5d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a more condensed way to achieve this (we use it in other places in the Foam codebase):
It's purely a matter of style, for me the advantage is that it allows to use
const
(a bummer that js/ts don't have a functionalswitch
statement), it's a bit more clear that the conditions are just about initiating the variable, and that the conditions can a bit more flexible. I think it takes a sec getting used to, but it flows once the eye is trained :)For consistency with the codebase I would lean towards using the pattern for both
extractor
andformatter
.see
link-completion.ts:188
andgenerate-link-references.ts:68
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for consistency! I can't help but unravel ternary operators in my head, which make chained ones seem complex, but you're right, they start looking just like an if/else block in time. Updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this I assume that if someone has both settings, the old setting will overwrite the new one?
I think that the new setting should always have the precedence, and we should only read the previous one if the new setting has not been used, wdyt?