Relax Alfalfa Point ID Requirements #5296
Conversation
TShapinsky
changed the title
TShapinsky
added
the
Pull Request - Ready for CI
TShapinsky
force-pushed
the
alfalfa_id_validation
branch
from
93af06f to
912124c
Compare
| bool AlfalfaPoint_Impl::isValidId(const std::string& id) { | ||
| return !id.empty() && boost::regex_match(id, boost::regex(R"(^[A-Za-z0-9_\-\[\]:()]*$)")); | ||
| } |
There was a problem hiding this comment.
@TShapinsky can you clarify what you meant by "onerous"?
Was it just annoying / too restrictive?
Or was is slow?
Boost (and std) regexes are awful in terms of performance, but using a static regex makes it better (we use Meyers singletons in IddRegex.hpp for eg)
There was a problem hiding this comment.
It was mostly annoying / too restrictive. For some background, the point ids become path parameters when interacting with the Alfalfa/BOPTest REST API. Originally I thought it would be neat to restrict the character set to avoid needing a lot of percent encoded characters. However, it's pretty common for names in open studio to include various different characters and since the requirement isn't hard I decided to get rid of it in the interest of making the alfalfa interface more user friendly.
src/alfalfa/AlfalfaPoint.cpp
Outdated
| bool AlfalfaPoint_Impl::isValidId(const std::string& id) { | ||
| return !id.empty() && boost::regex_match(id, boost::regex(R"(^[A-Za-z0-9_\-\[\]:()]*$)")); | ||
| } | ||
|
|
||
| std::string AlfalfaPoint_Impl::toIdString(const std::string& str) { | ||
| return boost::regex_replace(str, boost::regex(" "), "_"); |
There was a problem hiding this comment.
just a side note, but this is probably much faster, not that it matters much probably
#include <algorithm>
std::replace( str.begin(), str.end(), ' ', '_')
Edit: It is in fact, 150 times faster: https://quick-bench.com/q/LBF5Z_hhF14xxmjJ8IGs7UhGAUY
Co-authored-by: Julien Marrec <julien.marrec@gmail.com>
Co-authored-by: Julien Marrec <julien.marrec@gmail.com>
|
CI Results for d5c11a6:
|
|
@kbenne can you merge it? I don't have permission to do so. |
Pull request overview
After testing the new interface in the real world I found the requirements on id characters to be too onerous. This PR removes all requirements outside of non emptiness.
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.