-
Notifications
You must be signed in to change notification settings - Fork 2
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
Invalid Graph on Combobox #442
base: main
Are you sure you want to change the base?
Conversation
You should include the issue number in the description. This way we'll do a review to understand the motivation behind the update. Also try to be more specific in the commit name, not just "fixed" |
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.
This change solved the problem. But I think you should also change the source of the problem: filtering should not return a file with a blank last line. The same goes for inserting the usual new graph, which also inserts a blank line.
@atilaajones the blank line after filtering was an strategy proposed to fix the issue #397 . We can see the changes on the PR #403 |
I have observations about this PR:
Note Test if this problem is related to this PR to resolve it here
GraphFilter/source/domain/utils_file.py Lines 66 to 69 in 19e33e8
|
Test the reported behavior, then try to solve it in PR #444 |
I don't understand why that I need to make that changes on the change graph file PR |
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.
Working on macOS Sonoma version 14.1.1 Chip Apple M1
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.
@rosaigor138 The blank line at the end is still read by the software in ubuntu
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.
@fsoupimenta. I think this PR can be merged, right?
Closes #433