Skip to content

Conversation

@leotrs
Copy link
Contributor

@leotrs leotrs commented Aug 24, 2020

This is one of a series of PRs aimed at fixing #337. This PR does a few things:

  1. Introduces the tempconfig context manager. This works as follows:

       c = Camera()
       c.frame_width == config['frame_width']        # -> True
       with tempconfig({'frame_width': 100}):
           c = Camera()
           c.frame_width == config['frame_width']    # -> False
           c.frame_width == 100                      # -> True

    So it temporarily changes the config so that all classes that use it for default values will use the new (temporary) config. After
    the with statement ends, the config is restored. This accounts for the diff in config.py, __init__, and test_config.py. I remember @PgBiel in the past requested the feature of changing the global config. Now we can do it!

  2. The problem with the code above is that Camera instances used the value of frame_width that was available at the time when the Camera class was being defined, not the value of the config at the time that the instance was being constructed. So I had to remove a few keys from Camera.CONFIG and set them in Camera.__init__ instead. I did it in a way that it will later be easy to change to attrs cc @PgBiel. This accounts for the diff in camera.py and test_camera.py

  3. For some reason my changes made a difference in test_logging.py. I made it so that this test doesn't care about whitespace in the logs and now it works. I believe this is a fair change for now since apparently Refactored tests and added new tests tools for videos-tests. #335 is going to deprecate how tests currently work. cc @huguesdevimeux

Hope this makes sense! This is just the first in a few PRs concerning the config system.

leotrs added 7 commits August 23, 2020 23:27
…g dict at construction, and not when the Camera class is defined as it was previously done
… only differ in whitespace. Moreover, by the time that this PR is merged, the test_logging file will have been deleted by #335
@leotrs leotrs mentioned this pull request Aug 25, 2020
@huguesdevimeux
Copy link
Member

huguesdevimeux commented Aug 26, 2020

For some reason my changes made a difference in test_logging.py. I made it so that this test doesn't care about whitespace in the logs and now it works. I believe this is a fair change for now since apparently #335 is going to deprecate how tests currently work.

I think @Aathish04 is better to take a look at that, as he wrote these tests.

But personally, this seems very fine to me. (Nevertheless, #335 does not aim to change logging tests, not atm)

Copy link
Member

@huguesdevimeux huguesdevimeux left a comment

Choose a reason for hiding this comment

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

Your test fix is I think not good.
I think it's because the log_width parameters in the config is not right, as it should be set to 512.
One way to fix it would be to create a custom .cfg with log_width = 512 and to use it in the CLI in the logging test. That's what I did in #335

cc @Aathish04 (author of this test)

@huguesdevimeux huguesdevimeux added Changes requested and not yet done refactor Refactor or redesign of existing code labels Aug 27, 2020
@huguesdevimeux huguesdevimeux added pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! and removed Changes requested and not yet done labels Aug 28, 2020
@huguesdevimeux huguesdevimeux mentioned this pull request Aug 28, 2020
1 task
@huguesdevimeux huguesdevimeux self-requested a review August 28, 2020 16:20
@leotrs
Copy link
Contributor Author

leotrs commented Aug 28, 2020

Ok, fixed the tests thanks to @Aathish04.

Now we need one more approval.

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

Labels

pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! refactor Refactor or redesign of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants