Add new example #18
Add new example #18
Conversation
nagayev
commented
Dec 8, 2022
•
nagayev
commented
- New example based on this code.
- Link to docs at READE
README.md
Outdated
| @@ -20,6 +20,7 @@ To compile and install the shared library to your system, take the following ste | |||
| mkdir build | |||
| cd build | |||
| cmake .. | |||
| cd ../ | |||
There was a problem hiding this comment.
I don't think you want to cd back before calling make.
If you want to go back to the root directory, maybe you could try something like this?
mkdir -p build
pushd build
cmake ..
sudo make -j8 install
popdThere was a problem hiding this comment.
@project-eutopia Oh, it's strange for me, but you are right.
I was wrong.
docs/README.MD
Outdated
| @@ -0,0 +1,3 @@ | |||
| # Documentation | |||
|
|
|||
| Please visit [github pages](https://project-eutopia.github.io/vega/). | |||
There was a problem hiding this comment.
I noticed that we already link to this in the readme: https://github.com/project-eutopia/vega/blob/master/README.md#vega. Maybe it's not prominent enough though? I would be okay with you adding a new sub-heading that specifically just links to this documentation, but this docs folder is just for the Doxygen output (see HTML_OUTPUT value of the Doxyfile), so we shouldn't manually add any files to this folder.
There was a problem hiding this comment.
@project-eutopia Okey.
Maybe it's a good idea to add md5 files to .gitignore?
There was a problem hiding this comment.
The documentation in the docs folder is what is deployed to the Github pages you linked to here, so it should not be in the .gitignore.
There was a problem hiding this comment.
@project-eutopia I think files like this are useless.
There was a problem hiding this comment.
The entire docs folder is autogenerated from the codebase's Doxygen comments. The generated map and md5 files have a specific purpose: https://sourceforge.net/p/doxygen/mailman/message/10983184/. If you still think they are useless I would recommend filing a bug with their repository: https://github.com/doxygen/doxygen/issues.
No one should really be looking at the autogenerated raw files in that directory, just the documentation that is deployed from the docs folder at https://project-eutopia.github.io/vega/.
Regardless, such changes to the documentation should be done in a separate PR, because the stated purpose of this PR is just to add an code example.
There was a problem hiding this comment.
Could you either remove this file from the pull request or update the main README.md file to make the hyperlink to the documentation more prominent like I suggested above? Thanks
examples/edit.cpp
Outdated
| #include "vega/dicom/file.h" | ||
|
|
||
| int main(int argc, char *argv[]) { | ||
| if (argc < 3){ |
There was a problem hiding this comment.
I don't link to any specific style guide, but internally I have been trying to follow the Google C++ style guide.
https://google.github.io/styleguide/cppguide.html#Conditionals:
"put one space between the if and the opening parenthesis, and between the closing parenthesis and the curly brace"
examples/edit.cpp
Outdated
|
|
||
| int main(int argc, char *argv[]) { | ||
| if (argc < 3){ | ||
| std::cout<<"Usage: edit input_file output_file"<<std::endl; |
There was a problem hiding this comment.
Could you add spaces around the << operators?
(no specific rule about this in the style guide, but all code examples have spaces: https://google.github.io/styleguide/cppguide.html)
There was a problem hiding this comment.
One other note, could you add a comment about how to build this binary? This usage message assumes that the output binary will be named edit for instance.
examples/edit.cpp
Outdated
| vega::dicom::File file(argv[1]); | ||
|
|
||
| std::string& name = file.data_set()->element<vega::dictionary::PatientName>()->manipulator()->at(0); | ||
| name = "Smith^Alice"; |
There was a problem hiding this comment.
I like the idea of having an example which shows how to edit a file. Could you update this logic though to do some bounds checking? The element<T> method for instance could return a nullptr if there is no patient name, and the manipulator might not have a 0th element yet (the manipulator acts as a std::vector<std::string>, so you can either check that size() >= 1, or just loop through all the names and change them, maybe something like this:
auto element = file.data_set->element<vega::dictionary::PatientName>();
if (element) {
// If there are patient names, set them all to "Smith^Alice"
for (auto& name : element->manipulator()) {
name = "Smith^Alice";
}
} else {
// If there are no patient names, add one with value "Smith^Alice"
element = file.data_set()->new_element<vega::dictionary::PatientName>();
element->manipulator()->push_back("Smith^Alice");
}If you use this, please test this locally though in case I made any minor errors.
|
I just unresolved some comments that were resolved prematurely without changes being applied to this pull request. Could you please resolve comments after either making the changes suggested or responding to the comments. |
|
@project-eutopia I have resolved them on my desktop. |
|
@project-eutopia Please check my PR again. |
