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

Split .data folder #779

Merged
merged 4 commits into from
Feb 14, 2024
Merged

Split .data folder #779

merged 4 commits into from
Feb 14, 2024

Conversation

joaoandre-avaiga
Copy link
Collaborator

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add a new entry in the migrated Config?
There was only "storage_folder" before, do we need to add the "tp_storage_folder"

@@ -28,9 +28,12 @@ class CoreSection(UniqueSection):

Attributes:
root_folder (str): Path of the base folder for the taipy application. The default value is "./taipy/"
storage_folder (str): Folder name used to store Taipy data. The default value is ".data/". It is used in
storage_folder (str): Folder name used to store user data. The default value is ".user_data/". It is used in
Copy link
Member

Choose a reason for hiding this comment

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

How about "data_folder"?

Copy link
Member

Choose a reason for hiding this comment

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

This folder is the entrypoint folder for a user to get and drop data. I prefer data_folder instead of storage_folder, to be honest, but then it will be a breaking change. Am I correct?
What do you think?

For the default value, I have two remarks:

  1. Why not make the default value not start with a .. In the end, it is made to be accessed by the user. It is not a hidden folder.
  2. Why prefix it by user? Why not just data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This folder is the entrypoint folder for a user to get and drop data. I prefer data_folder instead of storage_folder, to be honest, but then it will be a breaking change. Am I correct?

Yes, I kept the config name the same to avoid a breaking change. My propose is to keep storage folder for the user data, so it doesn't have impact right now. We could change the name in 4.0, in the future.

Why not make the default value not start with a .. In the end, it is made to be accessed by the user. It is not a hidden folder.

I guess theres no reason to keep it hidden.

Why prefix it by user? Why not just data?

I think data may be a little to generic and could conflict with other things the user have. We do delete stuff from the data folder in some cases, so it may be more secure if it's not a common name.

conjunction with the *root_folder* attribute. That means the storage path is <root_folder><storage_folder>
(The default path is "./taipy/.data/").
(The default path is "./taipy/.user_data/").
tp_storage_folder (str): Folder name used to store Taipy data. The default value is ".taipy_data/". It is used
Copy link
Member

Choose a reason for hiding this comment

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

How about "entities_folder"?

Copy link
Member

Choose a reason for hiding this comment

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

Whatever the name of the config attribute, I would prefix it with taipy so it is explicit that it is for internal usage. Actually I would use taipy_folder directly, but I am ok with both taipy_storage_folder and taipy_entities_folder.
What do you think?

For the default value, I would go for .taipy instead of .taipy_data

(The default path is "./taipy/.user_data/").
tp_storage_folder (str): Folder name used to store Taipy data. The default value is ".taipy_data/". It is used
in conjunction with the *root_folder* attribute. That means the storage path is
<root_folder><storage_folder> (The default path is "./taipy/.taipy_data/").
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about this? I don't think the .taipy folder ever exists

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Im not sure. I kept the current text, just replaced .data with the new folder name. Maybe @jrobinAV can verify if this docstring is correct.

Copy link
Member

@jrobinAV jrobinAV Feb 5, 2024

Choose a reason for hiding this comment

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

<storage_folder> should be renamed by <taipy_storage_folder> (or whatever the name of the attribute we chose).

Suggested change
<root_folder><storage_folder> (The default path is "./taipy/.taipy_data/").
<root_folder><taipy_storage_folder> (The default path is "./taipy/.taipy/").

@@ -122,12 +122,13 @@ def __init__(
**properties,
)
self._path = properties.get(self.__PATH_KEY, properties.get(self.__DEFAULT_PATH_KEY))
if self._path and ".data" in self._path:
Copy link
Member

Choose a reason for hiding this comment

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

.data?
Can we at least put it as a const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only to migrate old taipy projects to the new structure. If .data is on the datanode current path, we move the file to the new folder. I can put it in a constant.

Copy link
Member

@jrobinAV jrobinAV left a comment

Choose a reason for hiding this comment

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

We need to converge on the config attribute names and their default values.

Otherwise, I believe it is good.

conjunction with the *root_folder* attribute. That means the storage path is <root_folder><storage_folder>
(The default path is "./taipy/.data/").
(The default path is "./taipy/.user_data/").
tp_storage_folder (str): Folder name used to store Taipy data. The default value is ".taipy_data/". It is used
Copy link
Member

Choose a reason for hiding this comment

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

Whatever the name of the config attribute, I would prefix it with taipy so it is explicit that it is for internal usage. Actually I would use taipy_folder directly, but I am ok with both taipy_storage_folder and taipy_entities_folder.
What do you think?

For the default value, I would go for .taipy instead of .taipy_data

@@ -28,9 +28,12 @@ class CoreSection(UniqueSection):

Attributes:
root_folder (str): Path of the base folder for the taipy application. The default value is "./taipy/"
storage_folder (str): Folder name used to store Taipy data. The default value is ".data/". It is used in
storage_folder (str): Folder name used to store user data. The default value is ".user_data/". It is used in
Copy link
Member

Choose a reason for hiding this comment

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

This folder is the entrypoint folder for a user to get and drop data. I prefer data_folder instead of storage_folder, to be honest, but then it will be a breaking change. Am I correct?
What do you think?

For the default value, I have two remarks:

  1. Why not make the default value not start with a .. In the end, it is made to be accessed by the user. It is not a hidden folder.
  2. Why prefix it by user? Why not just data?

(The default path is "./taipy/.user_data/").
tp_storage_folder (str): Folder name used to store Taipy data. The default value is ".taipy_data/". It is used
in conjunction with the *root_folder* attribute. That means the storage path is
<root_folder><storage_folder> (The default path is "./taipy/.taipy_data/").
Copy link
Member

@jrobinAV jrobinAV Feb 5, 2024

Choose a reason for hiding this comment

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

<storage_folder> should be renamed by <taipy_storage_folder> (or whatever the name of the attribute we chose).

Suggested change
<root_folder><storage_folder> (The default path is "./taipy/.taipy_data/").
<root_folder><taipy_storage_folder> (The default path is "./taipy/.taipy/").

<root_folder><storage_folder> (The default path is "./taipy/.user_data/").
tp_storage_folder (str): Folder name used to store Taipy data. The default value is ".taipy_data/". It is
used in conjunction with the *root_folder* attribute. That means the storage path is
<root_folder><storage_folder> (The default path is "./taipy/.taipy_data/").
Copy link
Member

@jrobinAV jrobinAV Feb 5, 2024

Choose a reason for hiding this comment

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

You mean <tp_storage_folder>.

@joaoandre-avaiga joaoandre-avaiga force-pushed the feature/split-data-folder branch 2 times, most recently from 65ba756 to 195d847 Compare February 11, 2024 23:34
@joaoandre-avaiga joaoandre-avaiga merged commit f217f7b into develop Feb 14, 2024
56 of 62 checks passed
@joaoandre-avaiga joaoandre-avaiga deleted the feature/split-data-folder branch February 14, 2024 01:53
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.

4 participants