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

Allow models to be iterable. #396

Merged
merged 1 commit into from
May 21, 2020
Merged

Conversation

ltrager
Copy link
Contributor

@ltrager ltrager commented May 15, 2020

No description provided.

@ltrager ltrager force-pushed the models_iter branch 3 times, most recently from a0a9e83 to 894782f Compare May 15, 2020 22:12
@ltrager
Copy link
Contributor Author

ltrager commented May 15, 2020

I'm not sure why pep8 is failing. None of the pep8 errors were related to changes in this branch. I fixed all but one of the errors. The last one looks fine to me.

@ajkavanagh
Copy link
Contributor

I'm not sure why pep8 is failing. None of the pep8 errors were related to changes in this branch. I fixed all but one of the errors. The last one looks fine to me.

I'm pretty sure it's due to a new version of the flake8 library being released and, as it's not pinned, it's throwing out code that used to be fine. I've been getting E741 errors ("ambiguous single letter variable") and E523 (IIRC - "wrong number of parameters in format()") (in other repos) that I've been fixing in various places.

It's a bit of a dilemma as otherwise innocent code starts failing due to "new" norms in the pep8 community around what code should look like, vs pinning to 'where we are now'.

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #396 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
+ Coverage   96.37%   96.38%   +0.01%     
==========================================
  Files          14       14              
  Lines        1075     1078       +3     
  Branches      127      128       +1     
==========================================
+ Hits         1036     1039       +3     
  Misses         18       18              
  Partials       21       21              
Impacted Files Coverage Δ
pylxd/models/_model.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7b489e...4b8c3ae. Read the comment docs.

@ltrager
Copy link
Contributor Author

ltrager commented May 19, 2020

I figured it out. The list of dictionaries was defined with using "[{" the new pep8 wants the list and dictionary definitions on different lines. For some reason it errors when you define the second dictionary in the list which made it confusing.

I agree pylxd should lock pep8 to a specific version however this branch does fix all the new pep8 errors :)

@ajkavanagh
Copy link
Contributor

I figured it out. The list of dictionaries was defined with using "[{" the new pep8 wants the list and dictionary definitions on different lines. For some reason it errors when you define the second dictionary in the list which made it confusing.

I agree pylxd should lock pep8 to a specific version however this branch does fix all the new pep8 errors :)

I'm not sure we should lock to a specific version? PEP8 is all about a consistent approach to code (a bit Black-lite, though, as it's not so opinionated!) For new commits to the library, I think it's acceptable for the flake8 to say "okay, but since you are updating, some other things aren't good and are possible errors." Even if those should be fixed in a separate PR, and the original PR is rebased on to that. Thus, generally, I'm leaning towards "don't pin flake8" but rather live with it or exclude certain rules that we don't care about.

@ajkavanagh
Copy link
Contributor

It might seem like a daft question, but why do we want models to be iterable?

@rockstar
Copy link
Contributor

I'm not sure we should lock to a specific version? PEP8 is all about a consistent approach to code (a bit Black-lite, though, as it's not so opinionated!) For new commits to the library, I think it's acceptable for the flake8 to say "okay, but since you are updating, some other things aren't good and are possible errors."

As the person who originally configured the lint gate, I tend to agree. Yeah, it's a little goofy that a flake8 update means you have to fix things unrelated to the patch. The way we've addressed this before is to just have a separate patch that addresses that update, and just wait for that to merge before merging this patch.

@ltrager
Copy link
Contributor Author

ltrager commented May 20, 2020

As suggested I broke out the PEP8 fixes into a separate pull request(#398). I'll rebase once that branch lands.

There are two use cases for making models iterable

  1. When interacting with pylxd via ipython its useful to see all the data returned without having to access each attribute individually.
  2. When an LXD Pod is added to MAAS the rack controller uses pylxd to retrieve the information and sends the data back to the region as commissioning output for processing. The commissioning output is JSON so the model has to be converted back to a dict. Right now MAAS is only sending back client.resources and client.host_info which are already dicts however this will be useful when we add network support.

If these use cases aren't inlign with the project feel free to reject this pull request. I just found it useful and thought I'd contribute it :)

@ajkavanagh
Copy link
Contributor

As suggested I broke out the PEP8 fixes into a separate pull request(#398). I'll rebase once that branch lands.

Excellent; just seen this and landed it.

There are two use cases for making models iterable

1. When interacting with pylxd via ipython its useful to see all the data returned without having to access each attribute individually.

2. When an LXD Pod is added to MAAS the rack controller uses pylxd to retrieve the information and sends the data back to the region as commissioning output for processing. The commissioning output is JSON so the model has to be converted back to a dict. Right now MAAS is only sending back client.resources and client.host_info which are already dicts however this will be useful when we add network support.

If these use cases aren't inlign with the project feel free to reject this pull request. I just found it useful and thought I'd contribute it :)

No, I think it's fine. Ergonomic helpers are always nice in the library.

Signed-off-by: Lee Trager <lee.trager@canonical.com>
Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

LGTM

@ajkavanagh ajkavanagh merged commit 653c7d6 into canonical:master May 21, 2020
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.

4 participants