Skip to content

Conversation

@rodrigogiraoserrao
Copy link

Limit of the foor loop was missing the last ngram of the text and hence missing a test that specifically addressed that.

Limit of the foor loop was missing the last ngram of the text.
@rodrigogiraoserrao rodrigogiraoserrao mentioned this pull request Mar 25, 2017
@rodrigogiraoserrao
Copy link
Author

This addresses the same issue as #412 that, for some reason, was reverted with #413 . 412 also introduced new tests that addressed the issue covered by the bugfix. When the 413 reverted the bugfix, those new tests started failing.

Flake8 was complaining about too much whitespace after a return keyword;
fixed that
@articuno12
Copy link
Contributor

@RojerGS I already fixed flake8 error in #418

@rodrigogiraoserrao
Copy link
Author

@articuno12 I have seen it. I only addressed this one because I hadn't noticed before.

@articuno12
Copy link
Contributor

@RojerGS A small advice, in open source, you should read what changes others have done before committing yours !

@rodrigogiraoserrao
Copy link
Author

@articuno12 you are most right, my apologies.

@rodrigogiraoserrao
Copy link
Author

@Chipe1 after your #430 I no longer know if I should fix the flake8 warnings like you asked in #416
Also, as @articuno12 notes, the work was already started by him/her. Should I do it nonetheless?

@Chipe1
Copy link
Contributor

Chipe1 commented Mar 29, 2017

@RojerGS #430 only removes a redundant recheck. Flake8 is still necessary for the build to pass. I think it would be better if there was a single PR that fixes all flake8 errors. Different PRs fixing the problem partially would be hard to track. The errors are very easy to fix and there is no point in breaking them into smaller components. I suggest you copy the changes which @articuno12 made and fix the other errors so that there would not be any merge conflicts. Or we can wait till @articuno12 fixes all the flake8 related problems in his PR

@articuno12
Copy link
Contributor

@Chipe1 I will fix them all in my PR.

@Chipe1
Copy link
Contributor

Chipe1 commented Mar 29, 2017

This is not the currect thread to discuss this but #415 adds fol_fc_ask which seems to be incomplete and causes flake8 errors. @articuno12 would either have to remove it, or @sofmonk needs to update the implementation.

@articuno12
Copy link
Contributor

@Chipe1 For now I have changed the lines causing flake8 errors in fol_fc_ask, this removes flake8 errors but doesn't guarantee fol_fc_ask to work.

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