-
Notifications
You must be signed in to change notification settings - Fork 114
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
Polish KeywordGenerator #4475
Polish KeywordGenerator #4475
Conversation
jenkins build this please |
f2ab222
to
e0b2490
Compare
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.
Looks fine, couple of suggestions
void write_file(const std::string& content, | ||
const std::string& file, | ||
const bool verbose, | ||
const std::string& desc = "source") |
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 think we can drop the default?
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 think we can drop the default?
True. I've pushed an update to that effect.
return fmt::format("#ifndef PARSER_KEYWORDS_{0}_HPP\n" | ||
"#define PARSER_KEYWORDS_{0}_HPP\n" | ||
"#include <opm/input/eclipse/Parser/ParserKeyword.hpp>\n" | ||
"namespace Opm {{\n" |
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.
may as well modernize, ie namespace Opm::ParserKeywords
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.
may as well modernize, i.e.,
namespace Opm::ParserKeywords
Agreed. I've pushed an update to that effect.
6b0af23
to
140aa96
Compare
jenkins build this please |
void write_file(const std::stringstream& stream, | ||
const std::string& file, | ||
const bool verbose, | ||
const std::string& desc = "source") |
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.
here as well?
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.
here as well?
😣
Thanks–I should have been more thorough before. I've pushed an update that I believe fixes that one as well...
Mostly by splitting long lines, removing needless declarations, and removing repeated 'std::endl' calls.
140aa96
to
430924b
Compare
jenkins build this please |
PR approved and build check is green. I'll merge into master. |
Mostly by splitting long lines, removing needless declarations, and replacing repeated
std::endl
calls by\n
characters where applicable.