Skip to content

Conversation

@Hartorn
Copy link
Member

@Hartorn Hartorn commented Oct 27, 2023

Description

Related Issue

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

@Hartorn Hartorn requested a review from rabah-khalek October 27, 2023 14:54
@Hartorn Hartorn self-assigned this Oct 27, 2023
cd ./install-test
sed -i 's/^\(python *= *\).*$/\1">=3.10,<3.12"/' pyproject.toml
poetry add "$(ls ../dist/*.whl)[server]"
poetry add "$(ls ../dist/*.whl)[server]"
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we still using poetry if there's PDM?

Copy link
Member Author

Choose a reason for hiding this comment

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

Test before was with poetry, and habits die hard ^^'
Can change, have to see how to create empty project with pdm

Copy link
Contributor

@andreybavt andreybavt Oct 27, 2023

Choose a reason for hiding this comment

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

I'd actually only use pdm to create a virtualenv (since pdm is already installed anyway)
pdm venv create XXX

and then activate it and install giskard with regular pip

pip install "$(ls ../dist/*.whl)[server]"
python -c 'import giskard'

Copy link
Contributor

Choose a reason for hiding this comment

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

this makes it way more readable too

Copy link
Contributor

Choose a reason for hiding this comment

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

and closer to what people will do

run: pdm build
- name: Create new project, install wheel and import
run: |
mkdir ./install-run
Copy link
Contributor

@andreybavt andreybavt Oct 27, 2023

Choose a reason for hiding this comment

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

I'd still skip the pdm init as we don't need a project to be create just simple venv:

pdm venv create --name test-install
eval $(pdm venv activate test-install)
pip install "$(ls ../dist/*.whl)"
python -c "import giskard"

after the second line in the script you shouldn't need to use pdm to run python

@Hartorn Hartorn force-pushed the try-import branch 4 times, most recently from b2aa512 to 1dcd305 Compare October 27, 2023 15:46
uses: snok/install-poetry@v1
- name: Create new project and install wheel
run: |
poetry new ./install-test
Copy link
Contributor

Choose a reason for hiding this comment

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

could you rename this step to "Create new project and install server wheel (Poetry)"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

sed -i 's/^\(python *= *\).*$/\1">=3.10,<3.12"/' pyproject.toml
poetry add "$(ls ../dist/*.whl)[server]"
poetry add "$(ls ../dist/*.whl)[server]"
- name: Create new project, install wheel and import
Copy link
Contributor

Choose a reason for hiding this comment

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

please also add Poetry to the step name

Copy link
Contributor

@andreybavt andreybavt left a comment

Choose a reason for hiding this comment

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

to make it clear what is going on let's mention pdm/pip/poetry in step names

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@andreybavt andreybavt marked this pull request as ready for review October 27, 2023 16:41
@andreybavt andreybavt requested a review from a team October 27, 2023 16:41
@andreybavt andreybavt merged commit 34595ef into main Oct 27, 2023
@andreybavt andreybavt deleted the try-import branch October 27, 2023 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants