-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[io] Remove non-user related doc from TFile class. [skip-ci] #9616
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
Conversation
|
Starting build on |
| ../../../tutorials/io/file.C | ||
| End_Macro | ||
| The structure of a directory is shown in TDirectoryFile::TDirectoryFile | ||
| A ROOT file is a file-system file storing objects, possibly inside directories. |
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 ROOT file is a file-system file storing objects, possibly inside directories. | |
| A ROOT file stores objects in a file-system-like logical structure, possibly including directory hierarchies. |
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.
No, it's a file-system file, really :-) Happy to see other wording suggestions that preserve the meaning :-)
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.
Better?
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 wanted to clarify the important point that a TFile will end up on your disk, as a regular file. I don't think your current wording gets that across: it talks about the content of a TFile.
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.
ooooh "a file-system file" means "it's a file in your file system", not "it's a file the content of which logically works like a file system"?
"An on-disk file" then?
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.
Sounds great!
bellenot
left a comment
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 trust you 😉
| A ROOT file is a suite of consecutive data records (TKey instances) with | ||
| a well defined format. | ||
| If the key is located past the 32 bit file limit (> 2 GB) then some fields will |
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.
This information is "essential" developer and advancer user information and belong in the repository (as it can and has evolved). There should be an explicit link or reference to where in the repository that information lives.
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.
That's referenced as \sa \ref rootio, see https://root.cern/doc/master/rootio.html
If it's not in there I will add it there; let me know where you want 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.
That's a bit obscure, especially when 'just' reading the source from vi. It probably should be clarified that the information is available/recorded(?) in ./io/doc/TFile and in particular tfile.md and tdirectory.md
( as a side note the 'format' in those file, both in the text version and the render version seems (to me) much harder to read than the one here (probably the missing/removed column 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.
as a side note the 'format' in those file, both in the text version and the render version seems (to me) much harder to read than the one here (probably the missing/removed column delimiter
Fine, but that shouldn't block this PR.
That's a bit obscure, especially when 'just' reading the source from vi.
Are you complaining about doxygen syntax being obscure, or you not knowing what's in these files? (Serious question.) Either way, my mission is to deconfuse users not necessarily Philippe, and this PR seems to be a step in the right direction ;-) There are several parts missing, e.g. TFile::Open instead of constructor etc - but that info wasn't there to begin with. So - what do we do?
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.
Fine, but that shouldn't block this PR.
In my opinion it was essential. This documentation is not just for me but also essential for future maintainer and for external implementer of the format. It could not be removed if there was not a (readable) alternative available. Glad-fully that was resolved in a separate PR.
Are you complaining about doxygen syntax being obscure, or you not knowing what's in these files? (Serious question.)
I am just stating that (possibly due to my missing in-brain doxygen parser) given those strings, I can not find the text file that contains the information. So maybe we can add outside the view of doxygen the actual name of the file or directory that contains the information (to help with browsing the code through a text editor) [As this information is "essential" to the implementation/documentation of the code, one would expect it to be close to the code ... or at least easy to reach.]
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.
Related to my last comment: doxygen/doxygen#9331
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.
When do they tag a release with that feature?
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.
The ROOT online-docs are created with a self-built version of doxygen. If I remember correctly, @couet has compiled doxygen versions from git-master that were not exactly at a specific doxygen release, so maybe he can just 'git pull' and recompile on the build machine.
The next stable release will be 1.9.5 probably in four or five months I guess.
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 I tested 1.9.5 on my machine first before updating the one on the build machine.
It seems stable I will soon update the build machine.
And yes, we are using the most recent version of doxygen.
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.
Done:
sftnight@root-ubuntu-2004-3:~/doxygen/build$ doxygen --version
1.9.5 (fdd8e8e750e1b8e58b4d9cc758293f532278dcca)
|
Build failed on mac11/cxx17. Failing tests:
And 4 more |
|
Hi, gentle reminder that we need this patch to fix the repeated text at https://root.cern/doc/master/group__IO.html 😄 |
pcanal
left a comment
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.
We are still missing a more 'human' readable reminder that the detail of the file format are in io/doc/TFile
|
I proposed this alternative: #14929 |
Also preparing for new IO group doc