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.

Sign up for GitHub

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

Jump to bottom

Removing typying dependency #85

Merged
merged 25 commits into from
Jan 31, 2024
Merged

Removing typying dependency #85

merged 25 commits into from
Jan 31, 2024

Conversation

JunCEEE
Copy link
Contributor

@JunCEEE JunCEEE commented Dec 11, 2023 •
edited
Loading

  • Add SimEx-Lite and McStasScript tests to the CI
  • Fix GitHub CI
  • Remove typying dependency

mads-bertelsen
mads-bertelsen reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@mads-bertelsen mads-bertelsen left a comment

Choose a reason for hiding this comment

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

We still have two dependency limits:
pint <= 0.19.2
dill <= 0.3.5.1

Are these still necessary?

Otherwise everything looks good!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
tests/integration/plusminus/requirements_dev.txt Outdated Show resolved Hide resolved
@JunCEEE
Copy link
Contributor Author

JunCEEE commented Dec 15, 2023

We still have two dependency limits: pint <= 0.19.2 dill <= 0.3.5.1

Are these still necessary?

Otherwise everything looks good!

They are actually necessary. pint <= 0.19.2 for pint.quantity. dill <= 0.3.5.1 for dump.

CFGrote
CFGrote reviewed Jan 12, 2024
View reviewed changes
.github/workflows/ci.yml
@@ -24,7 +25,26 @@ jobs:
pytest .
- name: Test McStasScript
run: |
python3 -m pip install McStasScript
git clone https://github.com/PaNOSC-ViNYL/McStasScript.git
Copy link
Contributor

Choose a reason for hiding this comment

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

could use pip install git+https://...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good suggestion, but here we need to run pytest in the test folder. Therefore it seems better to keep the current version.

libpyvinyl/BaseCalculator.py
@@ -49,7 +49,7 @@

class BaseCalculator(AbstractBaseClass):
"""
:class: Base class of all calculators.
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained during the meeting, the display before and after removing the :class: text shows that the text doesn't add important information but interrupts the layout. Therefore I decided to remove it.

libpyvinyl/BaseCalculator.py Outdated Show resolved Hide resolved
libpyvinyl/BaseFormat.py Show resolved Hide resolved
tests/integration/plusminus/requirements_dev.txt Outdated
Sphinx==3.5.2
twine==1.14.0
sphinx_rtd_theme==0.5.1
bump2version
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove the version numbers? does this work on multiple OS/platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the versions back.

@CFGrote CFGrote merged commit bb4cfc6 into master Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mads-bertelsen mads-bertelsen mads-bertelsen left review comments

@CFGrote CFGrote CFGrote approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JunCEEE @CFGrote @mads-bertelsen