-
Notifications
You must be signed in to change notification settings - Fork 271
Test Cleanup #1287
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
Comments
I agree 👍 |
I'm working on this and I had a few questions.
Should the |
I'd prefer it to be refactored but I don't think we have a "convention" yet for this particular example. Perhaps:
? |
So, just to be clear, this is fine
Then, wherever ? |
That's what I'm suggesting yes but I wouldn't be surprised if through discussion and review we come up with something much nicer... Essentially I'm saying, that's what I would go with but it might need to be changed. |
How about just using the general preference and its resulting namespace? e.g.
It's one fewer characters in the assertion, fewer import lines and consistent throughout. |
I think this is more simpler than having 2 |
Yup, @meatballs's suggestion is better 👍 (you might need to tweak an |
IMO it's also fine in this case to use
particularly if any one of the imported values is frequently used. FYI in Python, a.b.c incurs module lookups (dictionary lookups) each time it's invoked (since the module or its values could be dynamically overwritten). This is why people sometimes fix a reference outside of a loop:
instead of
This can be a surprising speed up in some cases. |
I'm fine with that as well 👍 |
I am not that proficient in python, so I'll refactor it the way you think will be efficient, since at the end of the day, the code maintenance should be easy. 😅 |
Sure, also I had a typo in the example, just fixed it |
@marcharper Great! . To summerize, I'll get started on refactor as :
Edit: fixed code highlighting |
great, thanks! |
I refactored the code, but on running
I tried some solutions SO, etc., but I can't seem to fix it. This article explains this problem, but I can't seem to get it. Can anyone give me some pointers? Edit: Fixed markdown |
Hi, I think it's either an installation issue or the tests are being run from the wrong directory. The output on the PR from Appveyor gives different (more expected) test failures. Try running ./test in the axelrod directory, which should run the test commands without having to install your modified library.
|
Closed by #1308 |
Uh oh!
There was an error while loading. Please reload this page.
Our test files are inconsistent with imports. For example to use the
Match
class in tests, various files use any one ofI believe the first is our general preference. It would be nice to update the test files and any others to be consistent.
The text was updated successfully, but these errors were encountered: