MycroftAI / padatious

A neural network intent parser
http://padatious.readthedocs.io
Apache License 2.0
159 stars 40 forks source link

PR for fixing issue #23 #24

Closed repodiac closed 4 years ago

repodiac commented 4 years ago

see https://github.com/MycroftAI/padatious/issues/23 for further details

pep8speaks commented 4 years ago

Hello @repodiac! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 73:101: E501 line too long (101 > 100 characters)

Comment last updated at 2020-05-25 09:49:15 UTC
repodiac commented 4 years ago
* In the file [`padatious/intent_container.py`](https://github.com/MycroftAI/padatious/blob/3c67790e21c716c15843f23ffefe0eb6f97ab64f/padatious/intent_container.py):

Line 76:101: E501 line too long (118 > 100 characters)

* In the file [`padatious/training_manager.py`](https://github.com/MycroftAI/padatious/blob/3c67790e21c716c15843f23ffefe0eb6f97ab64f/padatious/training_manager.py):

Line 51:17: E117 over-indented (comment) Line 52:17: E117 over-indented (comment)

I checked back with running autopep8 --in-place --aggressive --aggressive <file.py> on the two files above... nothing changed. Does the bot have an alternative representation of PEP8? Can someone help here?

forslund commented 4 years ago

Let me look at the pep-8 things.

I'm a bit perplexed over the conflicts in this PR... It's not like much has changed recently.

Can you try to rebase to resolve those?

repodiac commented 4 years ago

Ehm... can you guide me hjere a little what you exactly mean? I also haven't "rebased" anything so far (I have an idea what the git command means/does though), can you provide me with an example what I should try here? Thanks!

forslund commented 4 years ago

I'm not 100% sure how it's done when you're using the dev-branch for doing the changes but I think the following will do the trick Guessing the .../MycroftAI/padatious.git repo is called upstream

Fetch the latest dev from the upstream repo: git fetch upstream rebase your work on top of the latest upstream git rebase upstream/dev

if the rebase halts due to conflicts correct them add the files that were corrected git add FILE then continue the rebase git rebase --continue

Push the rebased branch to your repo git push -f origin dev

The line length is 100 for this repo, the indentation of the comments should probably be moved out one level (since they refer to the else if I'm reading this correctly)

repodiac commented 4 years ago

Hi, I manually changed the line length in intent_container. Can you merge now?

The guidelines you gave me did NOT WORK, how do I get the "upstream" von MycroftAI padatious and merge (?) it with my padatious repo? I pushed/rebased my own repo but that obviously did not help?

forslund commented 4 years ago

You can also use the Resolve conflicts button here in the PR to try to fix the issues using github's online tool.

If you want to add the upstream repo so you can sync against it: git remote --verbose will show you which remotes you have. I would guess you only have origin.

To add a new remote: git remote add upstream https://github.com/mycroftai/padatious

repodiac commented 4 years ago

Ok, I tried to use the online tool - still don't get what the issue actually was but anyway. Now github complains, it cannot rebase... of course, I do not have write access to MycroftAI/padatious - can you do it instead?

[remark: sorry for being such a PITA... I am not used to this/familiar with OS PRs :/ - thank you for your help and understanding]

repodiac commented 4 years ago

Ok, I guess I made it! Thank you so much for your help, it worked out exactly as you mentioned

forslund commented 4 years ago

I've read through this and it seems like it does what's intended (if I understand it correctly :) ). I'll clean up the history a bit and merge it. Many thanks for this contribution

repodiac commented 4 years ago

Great! Hope to see it in the master branch soon (still waiting for it :/