test suite reviews and discussions
 help / color / mirror / Atom feed
From: Owen Hilyard <ohilyard@iol.unh.edu>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: "Tu, Lijuan" <lijuan.tu@intel.com>, dts@dpdk.org
Subject: Re: [dts] [PATCH v1] python: standard project structure
Date: Tue, 24 Aug 2021 09:46:34 -0400	[thread overview]
Message-ID: <CAHx6DYCHXP3j0OXC==U4AvuBhvZf8AuZx3pCkKk-Q+iKJnaoQg@mail.gmail.com> (raw)
In-Reply-To: <1629804878-11074-1-git-send-email-juraj.linkes@pantheon.tech>

[-- Attachment #1: Type: text/plain, Size: 2375 bytes --]

>
> (and consequently remove the redundant dts shell script).


I don't think we should be making a breaking change to the external
interface right now. It may be worth it to discuss adding a deprecation
warning to the script, but we should give time for anyone who is using DTS
to using python directly. This includes the Community CI Lab, since this
change would break our current approach to updating DTS, which is to have a
"canary" system updated and running for a bit to find potential issues
before everything else gets updated.

1. Should we do the explicit import paths?


It may be easier to force absolute import paths since I know some older
automated tooling might now handle relative imports correctly. They also
make refactoring much easier if we ever need to move files around again.


> 2. If so, do we want to enforce it (for new patches)? How (is there a tool
> that check that or just manually in review)?


I think we should just to keep everything consistent throughout the
codebase. I don't know if there is a tool, but it should be easy to look at
the top of files in a patch and check for relative imports.


> 3. Some paths might not have been moved to the explicit format, as I
> may have missed them.
> 4. I did not test the patch as I don't have access to an environment
> in which I could do so. Is there a way to test it locally (i.e. without
> any hw dependencies)?


The easiest way would probably be to put something like this in main to
test:

from framework import *
from framework.flow import *
from framework.ixia_network import *
from tests import *
from nics import *

This should import every single file in the project, which will cause every
file to evaluate its own imports. As long as nothing imports main, that
should work.

5. Do we want to sort imports?


I think that we should ask that imports are logically grouped, but not
prescribe any particular method unless we start to have issues.


> 6. If so, how? Do we want to do it in this patch?


It might make sense to do a single pass now using isort (
https://github.com/PyCQA/isort). We can discuss adding that as a tool
later, but unless there's an easy way to add it as a commit hook, it might
make sense to do it once and leave it be. DTS has a lot of other stuff to
work on, and I think that this is something we can discuss once we've fixed
some of the bigger issues.

[-- Attachment #2: Type: text/html, Size: 3694 bytes --]

  reply	other threads:[~2021-08-24 13:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 11:34 Juraj Linkeš
2021-08-24 13:46 ` Owen Hilyard [this message]
2021-08-27  9:19   ` Juraj Linkeš
2021-08-27 13:03 ` [dts] [PATCH v2] " Juraj Linkeš
2021-09-06  3:01   ` Tu, Lijuan
2021-09-09 11:05     ` Juraj Linkeš
2021-09-10 12:31       ` Juraj Linkeš
2021-09-10 13:41   ` [dts] [PATCH v3] " Juraj Linkeš
2021-09-16  2:33     ` Tu, Lijuan
2021-10-07 11:26     ` [dts] [PATCH v4] " Juraj Linkeš
2021-10-19 12:51       ` [dts] [PATCH v5] " Juraj Linkeš
2021-10-22  8:28         ` Tu, Lijuan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHx6DYCHXP3j0OXC==U4AvuBhvZf8AuZx3pCkKk-Q+iKJnaoQg@mail.gmail.com' \
    --to=ohilyard@iol.unh.edu \
    --cc=dts@dpdk.org \
    --cc=juraj.linkes@pantheon.tech \
    --cc=lijuan.tu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).