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
feature/#397 scenario duplication #2373
base: develop
Are you sure you want to change the base?
feature/#397 scenario duplication #2373
Changes from 10 commits
9fe22e3
aec8bbe
c034db4
2df565c
3303259
f7ef682
3a3dbe6
a70b107
5b53c5f
ac0e57e
4d5f118
ffeb1c8
1b473a4
acaa0c1
8c69b76
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.
Please check your automatic formatter as this line is over 120 chars. Please revert the change.
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.
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 have 2 questions.
What would be the new file name if a data node is duplicated twice?
Similarly, if a duplicated data node is duplicated (dn -> duplicated_dn -> duplicated duplicated_dn)?
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.
yep so take
example.csv
as the file name, after you first duplicate the dn, it will create a new fileTAIPY_CLONED_new_dn_id_example.csv
. But if you want to duplicate this newly duplicated dn, another file will be created with the name pattern beingTAIPY_CLONED_another_new_dn_id_example.csv
. TheTAIPY_CLONED
prefix won't be repeated.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 see. So what about duplicating a data node twice?
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.
then we will have
TAIPY_CLONED_dn_duplicated_id_1_example.csv
for the 1st dn, and for 2nd dn, we will haveTAIPY_CLONED_dn_duplicated_id_2_example.csv
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 believe having both the prefix
TAIPY_CLONED
and the unique ID in the name is not necessary. If we can make the file name unique with the new ID, what is the purpose of keeping the prefix? We can probably propose something slightly better differentiating the case where the file name is generated by taipy or if it is provided by the user.If the file of the initial dn is generated (
dn.is_generated
), we can simply generate a new name for the duplicate with the same function (dn._build_path
). Otherwise, we can use your proposal without the prefix:{new_id}_{base_name}
.What do you think?
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.
Well because I think the prefix will make the naming clear as to what it is, just the id is fine but we will have to state clearly in the doc that this is what we generated. While they can understand it just from the prefix without doc.
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.
Is this repetitive?
Can we put this in the parent
_FileDataNodeMixin
class and override it when necessary?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.
hmmm I think it's a yes and no answer. The
self._properties
attribute doesn't exist in_FileDataNodeMixin
, but since ExcelDN, etc. are implementing_FileDataNodeMixin
, it can access it. But the syntax feels off for me, and since_FileDataNodeMixin
is a Mixin, I'm not comfortable puttingself._properties
in it. (I understand that we do have similar things fordef path(self, value)
(setter) in_FileDataNodeMixin
, but I'm still not sure if it's the best way to do 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.
I understand your point. However, other methods in the Mixin class doesn't seem to fit as well.
I think we did discuss this once. @jrobinAV Let us know what you think 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.
Yes, this is debatable. I would personally do the same as the path setter for consistency.