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.

Sign up for GitHub

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

Jump to bottom

Fix apy modules tests - Re: #516 #522

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix apy modules tests - Re: #516 #522

wants to merge 9 commits into from

Conversation

tomasff
Copy link

@tomasff tomasff commented Dec 20, 2019

This pull request attempts to fix issue #516.

Two changes were made:

  1. In the apy module a specific value stored in the constant SYNTAX_ERROR is returned if a syntax error occurred. This change was made so that several functions in this module can be tested again. (Related to changes in Display error messages due to wrong usage to users instead of admins #504)
  2. A new function check_syntax_error was created to test if a function returns the appropriate SYNTAX_ERROR code.

Althought this is not a very elegant solution it should allow us to test for any problems in this module when changes occur, while a more robust solution with maybe more specific exceptions isn't implemented.

@Androbin
Copy link
Collaborator

Maybe we could differentiate between LINE_ERROR, CMD_ERROR, TEXT_ERROR and so on?
LGTM otherwise 👍

@tomasff
Copy link
Author

tomasff commented Dec 20, 2019

@Androbin Sure, that's fine. Should I maybe create an enum or a dictionary?
The dictionary could be as follows:

errors = {
    'SYNTAX_ERROR': {
        'LINE_ERROR': 1,
        'CMD_ERROR': 2,
        'TEXT_ERROR': 3
    }
}

Or with an enum:

class SyntaxError(Enum):
    	LINE_ERROR = 1
        CMD_ERROR = 2
        TEXT_ERROR = 3

I could also maintain the current structure of adding constants at the top. However the dictionary seems more flexible, in my opinion. Could you please let me know which solution would be best?

@Androbin
Copy link
Collaborator

Nice writeup! I would argue for the enum variant. I think this best conveys the idea of getting exactly one of a known set of return values. Actually, I think we're also missing an OK value or similar.

@Androbin
Copy link
Collaborator

I do have to say, this actually looks quite suspicious:

text, text_error = strict_check(r'.*', input.group(1), apertium_identlang)

Actually, after closer inspection, I can't tell why these error names were picked originally.

@Androbin
Copy link
Collaborator

How about KEYWORD_ERROR for unknown sub-commands or similar, ENUM_ERROR for unknown source/target language or similar, and SYNTAX_ERROR for something that is closer to syntax errors in the classic sense like bad delimiters and stuff?

@Androbin
Copy link
Collaborator

For example, a strict_check on pairRE = langRE + '-' + langRE might lead to either SYNTAX_ERROR if there's not exactly one dash, or to ENUM_ERROR in case the language is unknown.

@tomasff
Created enum for functions status
4fb6641 
Created an enum for functions status.
Renamed error variables to a more appropriate name.
@tomasff
Copy link
Author

tomasff commented Dec 21, 2019

@Androbin is this what you meant?

wei2912
wei2912 reviewed Dec 21, 2019
View reviewed changes
Copy link
Member

@wei2912 wei2912 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose there's more work to be done to integrate the other error types into the rest of the code? :P I only see SYNTAX_ERROR and WEB_RESPONSE_ERROR being used.

modules/apy.py Outdated Show resolved Hide resolved
modules/test/test_apy.py Outdated Show resolved Hide resolved
@tomasff
Copy link
Author

tomasff commented Dec 21, 2019

@wei2912 I can integrate the other error types, however this would envolve removing a lot of the thrown GrumbleError. Is that alright?

@tomasff
Copy link
Author

tomasff commented Dec 21, 2019

@wei2912 @Androbin
Update: I tried integrating other errors types by replacing GrumbleError's with the new Status codes.

Unfortunately this broke some of the tests and I wasn't able to fix them.

In my opinion a good separate issue/ticket would be to think about a solid structure for error handling thats consistent across all modules, and temporarily fix the apy module's tests, so that we can verify if new changes break the module.

Please let me know if I should push my attempt to replace GrumbleError's with the new Status codes.

@Androbin
Copy link
Collaborator

The APY tests are green now. I agree with @tomasff
@wei2912 We should merge this now and have him submit another PR.

@wei2912
Copy link
Member

wei2912 commented Dec 22, 2019

IMO I don't think it's necessary to standardise the error reporting across all the modules. In nearly any other codebase I'd agree with the need for consistent error handling, but for a modular scripts system that Begiak has, I don't think it'd lead to much payoff. We could have each module handle exceptions as best as they can (since each module would have their own exceptions to deal with), and if they're unable to do so, the bot will attempt to catch the exception and report it unless it leads to an unrecoverable error.

As for replacing GrumbleError with the new Status codes, are you speaking of modifying other modules? That can be submitted in a separate GCI task and PR.

This is related to #509.

@wei2912
Copy link
Member

wei2912 commented Dec 22, 2019

I'll be putting this into the commit message, let me know if it summarises the details well:

  1. An enum was created to deal with different types of errors within the module, so that the tests can be fixed. (Related to changes in Display error messages due to wrong usage to users instead of admins #504.)
  2. A new function check_error was created to test exception handling.

wei2912
wei2912 requested changes Dec 22, 2019
View reviewed changes
modules/test/test_apy.py Outdated Show resolved Hide resolved
@tomasff
Copy link
Author

tomasff commented Dec 22, 2019

@wei2912 I meant replacing the use of GrumbleError in handle_error and check_no_data. I tried doing this, but a lot of tests related to the translate function broke and I wasn't able to fix them. I will try again today. Should I create an issue for this?

I think a standerdise way of handling errors would be benefitial because as of right now, errors are all handled a bit differently in every module: we're using the Status codes in the apy module and GrumbleError's in other modules.

@wei2912
Copy link
Member

wei2912 commented Dec 22, 2019 via email

@wei2912
Copy link
Member

wei2912 commented Dec 22, 2019 via email

@tomasff
Copy link
Author

tomasff commented Dec 24, 2019 via email

@wei2912
Copy link
Member

wei2912 commented Dec 28, 2019

@tomasff have there been any updates ever since your last comment? :P

@tomasff
Copy link
Author

tomasff commented Dec 29, 2019 via email

@wei2912
Copy link
Member

wei2912 commented Feb 20, 2020

@tomasff Hi, I just realised this PR hasn't been merged yet... how's the progress on this? :P

@tomasff
Copy link
Author

tomasff commented Feb 21, 2020

@wei2912 Sorry for the delay, I've been busy these past weeks. I'll try to finish it by next week.

@wei2912
Copy link
Member

wei2912 commented Feb 21, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wei2912 wei2912 wei2912 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomasff @Androbin @wei2912