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.

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

Control caching based on response #99

Merged
merged 7 commits into from Mar 29, 2019

Conversation

volodymyrss
Copy link
Contributor

I might be missing entierly the intent of the developers, and possibly there is a much better solution to my problem (#91).
This is just what I implemented which is sufficient for my needs, I wonder if it makes any sense.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 79.94% when pulling 50520b9 on volodymyrss:control_with_response into 6140d2c on sh4nks:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 79.94% when pulling 50520b9 on volodymyrss:control_with_response into 6140d2c on sh4nks:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 79.94% when pulling 50520b9 on volodymyrss:control_with_response into 6140d2c on sh4nks:master.

@coveralls
Copy link

coveralls commented Mar 15, 2019

Coverage Status

Coverage increased (+0.02%) to 79.94% when pulling a5e54f6 on volodymyrss:control_with_response into 6140d2c on sh4nks:master.

@sh4nks
Copy link
Collaborator

sh4nks commented Mar 20, 2019

This looks good to me from a first look! I'll take a closer look when I am home from work.

tests/test_view.py Outdated Show resolved Hide resolved
Co-Authored-By: volodymyrss <volodymyrss@users.noreply.github.com>
@volodymyrss
Copy link
Contributor Author

thanks for the suggestion!
I see you do not use print even in the test, I should have followed that. Could I ask why, for educational purposes?

@sh4nks
Copy link
Collaborator

sh4nks commented Mar 25, 2019

Wait, you were able to commit my suggested changes directly from github? 😄

Generally speaking, instead of using print statements, the information should be logged using a dedicated logger. The logger can then be configured to only print certain log levels to the console, or log everything to a file.

In our case, by reading the test cases you have written one can easily guess what the result is -- and in case the tests fail, pytest will show us all the needed information anyway.

Hope that clears things up!

@volodymyrss
Copy link
Contributor Author

Ok, I see, thanks!
I would usually use logger except for the tests, but then no reason to treat tests as second-class code.

Yeah, the technology of github is growing beyond our imagination. There was just a button to accept the suggestion, first time I see such a feature.

@sh4nks sh4nks merged commit 01b764e into pallets-eco:master Mar 29, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants