-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fixes for Issues #8 and #9 #49
Conversation
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.
LGTM with nit-picks: can you check my two comments and update?
.gitignore
Outdated
|
||
tutorial/conf.py | ||
test.mmd |
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.
Nit-pick: I wouldn't ignore the test.mmd
file, which seems to be kind of random...
return self.sequence | ||
except Exception: | ||
logging.warning( | ||
"\nCould not execute model. Make sure that model is:\n1. Formatted correctly.\n2. File ends with .xml or .json." | ||
) | ||
|
||
def validate_edge_cases(self): |
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.
Can you add a comment/docstring here that describes the edge cases that you are fixing?
Perhaps, it makes sense to have a more specific function name, because an edge case could be just about anything...
Removed some unnecessary igonores
* Small changes based on review comments * Linting issue * lint * lint * lint --------- Co-authored-by: MarcusRostSAP <[email protected]>
This should fix Issue #8 and #9.
The approach is quite naive and implements a for-loop with O(n) to fix the edge-cases mentioned in the Issues.
Let me know if a more in-depth solution is wanted!