NatLabRockies / NatLabRockies/OpenStudio
OSArgument of type Path: incomplete and confusing implementation
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 646
- Forks
- 237
- Avg merge
- 3d 11h
- Merged PRs (30d)
- 10
Description
Issue overview
The OS SDK and BCL-gem implementation of the OSArgumentType::Path arguments is incomplete, and neither handle the makePathArgument arguments:
isRead- what does that even mean?
- Is this assuming that this is for reading meaning it must be 1) a file (not a directory) and 2) it must exist?)
extension
Current Behavior
They are confusingly named, and not written to the measure.xml
Expected Behavior
Clarify the meaning of these. And ensure they get written to the measure.xml
Steps to Reproduce
here is an example measure to play with.
openstudio measure -u . produces a measure.xml with no extension or is_read.
Possible Solution
Ideally I think there should be 3 possible types of Path Arguments:
- A directory, no extensions specified
- A file that will be read and must exist. The extensions should be optional
- A file that will be written and does not have to exist. The extensions should be optional
I think the extensions should rather be specified as a kind of std::map<std::string, std::string>. And should be sanitized to a common format (eg check there is / there isn't a leading dot . and /or * or not) . for eg
// Display Name, extensions
"CSV Files", "*.csv"
"Excel Files", "*.xlsx *.xls"
Or maybe even a std::map<std::string, std::vector<std::string>>, such as an entry would be {"Excel Files", {"xlsx", "xls"}})
We must be able to differentiate from a Path of type directory and a Path of type file. Several options:
- Make a new OSArgumentType::DirectoryPath
- Ensure the extension is always set for the file ones, eg:
static OSArgument makePathArgument(const std::string& name, bool openForReading, const std::map<std::string, std::string>& extensions, bool required = true) {
if (extensions.empty()) {
m_extensions = {"All Files", "*"};
} else {
m_extensions = extensions;
}
}
static OSArgument makeDirectoryArgument(const std::string& name, bool required = true) {
m_extensions.clear()
}
- Add a bool isDirectory on the OSArgument class and modify the makePathArgument to take it in.
A potential measure.xml would look like
<arguments>
<argument>
<name>output_path</name>
<display_name>Output Path on Disk</display_name>
<type>Path</type>
<required>true</required>
<model_dependent>false</model_dependent>
<is_read>true</is_read>
+ <is_directory>false</is_directory> // May not be needed?
+ <extensions>
+ <extension>
+ <description>Excel Files</description>
+ <ext>xlsx</ext>
+ <ext>xls</ext>
+ </extension>
+ <extension>
+ <description>All files</description>
+ <ext></ext>
+ </extension>
+ </extensions>
</argument>
</arguments>
Further areas of improvements
If an existing file is what's wanted, the validateUserArguments method should probably catch the case where it doesn't exist.
Details
Environment
Some additional details about your environment for this issue (if relevant):
- Platform (Operating system, version): all
- Version of OpenStudio (if using an intermediate build, include SHA): 3.9.0 alpha
Context
Trying to implement it in OS Application at https://github.com/openstudiocoalition/OpenStudioApplication/pull/761
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with src/measure/OSArgument.hpp around lines 131-133 and trace makePathArgument in the SDK and BCL-gem implementations, then inspect the measure.xml generation and validateUserArguments. Use the linked example and openstudio measure -u . to compare current output; done means the Path semantics are agreed and is_read and extensions are represented consistently in measure.xml.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend-api-design, tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100