NatLabRockies / NatLabRockies/OpenStudio

OSArgument of type Path: incomplete and confusing implementation

Open
#5,273 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

component - Measures severity - Normal Bug
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:

https://github.com/NREL/OpenStudio/blob/f63cae5fbc076168a8d2acbd0d756998fde2a78e/src/measure/OSArgument.hpp#L131-L133

  • 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.

example_path_argument.zip

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:

  1. Make a new OSArgumentType::DirectoryPath
  2. 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()
} 
  1. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.