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

chore(test): Tidy up python tool #263

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

Callisto13
Copy link
Member

What this PR does / why we need it:

Only the second commit does anything new, the rest is tidying up.

The main purpose of this PR is to fix the e2e nightly build. I think I must
have messed up a rebase or something because I had run the exact
command so many times and it was fine so 🤷‍♀️ .

I also did a bit of tidying up in the test/tools/ dir. i have no idea how
a python project should be structured (even a toy one like this) but it was a
bit messy in there and there is much room for refactoring and tests so
organising things better now felt sensible.

I don't know whether this is the correct way to structure a python
project, but it was getting a bit messy in that dir so I have moved
things around. Also the Welder package can probably be refactored one day.
@Callisto13 Callisto13 added the kind/bug Something isn't working label Nov 15, 2021
Copy link
Contributor

@yitsushi yitsushi left a comment

Choose a reason for hiding this comment

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

Small style suggestion, not a blocker. lgtm

@@ -8,7 +8,7 @@
class Config:
def __init__(self):
self.dir = dirname(abspath(__file__))
self.base = dirname(dirname(dirname(abspath(__file__))))
self.base = dirname(dirname(dirname(dirname(abspath(__file__)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe with pathlib.Path it's more readable:

from pathlib import Path
Suggested change
self.base = dirname(dirname(dirname(dirname(abspath(__file__)))))
self.base = str(Path(abspath(__file__)).parent.parent.parent.parent)

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, parents[4] should work too, but then we have to check the length first

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe with pathlib.Path it's more readable

maybe, i will add that with my next pr in all the places we do this 👍

@Callisto13 Callisto13 merged commit 17de35e into liquidmetal-dev:main Nov 16, 2021
@Callisto13 Callisto13 deleted the fix-nightly branch November 16, 2021 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants