-
Notifications
You must be signed in to change notification settings - Fork 7
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
Format uk #50
Format uk #50
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #50 +/- ##
=====================================
Coverage ? 0.00%
=====================================
Files ? 69
Lines ? 18251
Branches ? 0
=====================================
Hits ? 0
Misses ? 18251
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@NeonMariia I think these are the users you said could review this: @robinhad @VicGrygorchyk . I can't request reviews from non-maintainers, but anyone should be able to leave a review with comments. If these changes look good to a native speaker, they are already passing unit tests so one of the maintainers here can merge it |
Hi @NeonDaniel ! |
Associated PR for parsing: #51 |
as mention in companion PR #51 we just had a stable release and are back in alpha versions, I am ok with merging this as is and fix any issues in follow up PRs. This is great work and I'd like to get it in the wild! Only see some debug prints to remove and maybe a rebase, there seems to be some duplicated code too between this and #51 |
Hi! Added a couple of comments with suggestion how to improve, overall looks great! |
It looks like there are a few unaddressed comments, but n approving review from @robinhad. Should there be an issue to include those changes later, or are then intentionally not included? I don't necessarily want to hold the PR for minor improvements but if they're valid, creating an issue will make sure we don't lose track of them. |
Hi @NeonDaniel, they are valid, please include them in separate issue as improvements |
couple changes
Looks like another PR got most of them so I'll open an issue for the last couple before merging this one. |
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.
leftover file
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.
that whole directory, on review
On final review before merging, there only turned out to be one unresolved addition to the resource files, so I just threw that in. I also found one more stray tempfile directory, so I've removed that and re-requested @emphasize's review so as not to self-merge. Should be good to go. |
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.
All unittests are commented out, i don't think this is wanted
No description provided.