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(#1602): discover depends on place #2432
feat(#1602): discover depends on place #2432
Changes from all commits
81d587f
a91c3ea
39c2c2c
ad4fc7f
4b24715
ace422d
60d4498
aff4fa6
2493053
449a1d7
e08d5eb
e18b782
5451f12
a36381b
96d4a1b
6f3a456
b648616
a7cca03
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.
@maxonfjvipon Is it a
tojo
? Maybe we can name it as a "default object name", or "default version"?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.
@volodya-lombrozo it's an identifier of tojo
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.
@maxonfjvipon But what is the identifier of tojo in this context?
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.
@volodya-lombrozo just a name of it as string
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.
@maxonfjvipon You treat it as an object name with hash, or I'm wrong?
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.
@volodya-lombrozo with or without. But if I wrap
tojo.identifier()
intoOnDefault
somewhere before this function, I'll not be able check if it contains hash or not. The check should happen in this function, that's why I pass tojoString
not asObjectName
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.
@maxonfjvipon I believe this change should be done in a separate PR. Please take a look that issue:
#2420
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.
@volodya-lombrozo I'm not sure in such particular case.
OnDefault
is just one moreObjectName
without complex behaviour.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.
@maxonfjvipon From the issue description:
We have the same
split
function among many classes (The same we can say aboutDELIMETER
static field). Hence, I believe this issue exactly about this.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.
@volodya-lombrozo I suggest to extend the #2420 so it affected this class too
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.
@volodya-lombrozo I believe adding
OnDefault
class is not about solving the problem #2420. It's just one more object name. Yes, with code duplication, that will be somehow resolved after solving #2420.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.
@maxonfjvipon Let's do it for now. But I would recommend to solve #2420 issue first and then return to that PR.
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.
@volodya-lombrozo yes, let's leave this as it. Object versioning has higher priority than refactoring at the moment
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.
@maxonfjvipon Why do we need this overcomplication? I didn't find any usage of the next constructor:
And to be honest, I don't see the reason for it.
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.
@volodya-lombrozo here. Scalar is needed because explicit
new OnDefault(tojo).hash()
may fail withIndexOutOfBounds
exception if tojo does not contain delimiter|
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.
@maxonfjvipon I see, thank you. Maybe it's better to let it fail as fast as possible? Otherwise it looks like you are trying to swallow the exception or use exceptions for flow control (both of the approaches don't lead to a good design).
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.
@volodya-lombrozo It should not fail at this step at all. Pay a bit more attention to the code:
If condition
tojo.contains(OnReplaced.DELIMITER)
is FALSE, the first object will be never evaluated, the second one will be -replaced
. But we need to construct it here and it should now fail on the construction stepThere 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.
@volodya-lombrozo maybe we will add one more object name for such cases, but there's only one for now, so there's no need