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.

Sign up for GitHub

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: add cliskTraceFile metadata to clisk trace files #1033

Closed
wants to merge 1 commit into from
Closed

feat: add cliskTraceFile metadata to clisk trace files #1033

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libs/ReactNativeLauncher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,9 @@ class ReactNativeLauncher extends Launcher {
await client.save({
_type: 'io.cozy.files',
type: 'file',
metadata: {
cliskTraceFile: true
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what to think about it.

First: It should be documented if used
Second: I don't think that we should add whatever we want to the io.cozy.files. The doctype is already pretty big and complex. Adding one more metadata make it mare complex.
Third: Since this is only for debug purpose, I don't think it should be added to the doctype
4th: I understand the idea, but once you have the regex done based on the name, it's enough now? Why do you consider the filename as a weak assumptions?

@paultranvan your thoughts on that?

Copy link
Contributor Author

@doubleface doubleface Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nothing prevents users to create or import files with the same name in their drive.
If the criteria is a metadata, it is impossible to make a mistake when we remove the trace files.

Copy link
Contributor

@paultranvan paultranvan Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From my external perspective, that sounds kind of hacky, and this feeling is worsen by the lack of doc 😄
I think we should prevent ourselves to add undocumented custom attributes in doctypes, particularly for io.cozy.files that are already very big and shared by many clients 🙏
At least, if you really need those kind of attributes, I think it would be much more nicer to group them into a metadata.konnector object.
The metadata object is already quite a mess, and I am pushing for grouping all scope-related attributes in a dedicated object. See the qualifications for instance.

data:
`<!-- ${url} -->
` +
Expand Down