Skip to content
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

Merged
merged 17 commits into from
Oct 12, 2023
Merged

Fixes for Issues #8 and #9 #49

merged 17 commits into from
Oct 12, 2023

Conversation

MarcusRostSAP
Copy link
Contributor

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!

Copy link
Collaborator

@TimKam TimKam left a 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
Copy link
Collaborator

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):
Copy link
Collaborator

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

MarcusRostSAP and others added 2 commits October 12, 2023 13:43
Removed some unnecessary igonores
* Small changes based on review comments

* Linting issue

* lint

* lint

* lint

---------

Co-authored-by: MarcusRostSAP <[email protected]>
@MarcusRostSAP MarcusRostSAP requested a review from TimKam October 12, 2023 12:20
@TimKam TimKam merged commit 4003c88 into signavio:main Oct 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants