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

Add lancium site adapter #267

Merged
merged 16 commits into from
Nov 24, 2022
Merged

Conversation

giffels
Copy link
Member

@giffels giffels commented Nov 11, 2022

This pull request adds support for the Lancium Compute site utilizing the aiolancium client.

P.S.: Due to running generate_apidoc.sh some additional rst files in the documentation have been updated.

@giffels giffels added the enhancement New feature or request label Nov 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2022

Codecov Report

Base: 98.85% // Head: 98.88% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (8fa3c3d) compared to base (7686dbb).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 8fa3c3d differs from pull request most recent head aa48d4b. Consider uploading reports for the commit aa48d4b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage   98.85%   98.88%   +0.03%     
==========================================
  Files          55       56       +1     
  Lines        2262     2325      +63     
==========================================
+ Hits         2236     2299      +63     
  Misses         26       26              
Impacted Files Coverage Δ
tardis/adapters/sites/lancium.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2022

This pull request introduces 2 alerts when merging fed0620 into 459f3de - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2022

This pull request introduces 2 alerts when merging 35ef591 into 459f3de - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2022

This pull request introduces 2 alerts when merging d950709 into 459f3de - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

tardis/adapters/sites/lancium.py Show resolved Hide resolved
logger.debug(f"{self.site_name} has status {resource_status}.")
resource_attributes.update(updated=datetime.now())
return convert_to_attribute_dict(
{**resource_attributes, **self.handle_response(resource_status)}

Check failure

Code scanning / CodeQL

Wrong number of arguments in a call

Call to [method SiteAdapter.handle_response](1) with too few arguments; should be no fewer than 3.
Copy link
Member Author

Choose a reason for hiding this comment

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

False positiv due to using partial.

tests/adapters_t/sites_t/test_lancium.py Fixed Show fixed Hide fixed
tests/adapters_t/sites_t/test_lancium.py Fixed Show fixed Hide fixed
tests/adapters_t/sites_t/test_lancium.py Fixed Show fixed Hide fixed
tests/adapters_t/sites_t/test_lancium.py Fixed Show fixed Hide fixed
tests/adapters_t/sites_t/test_lancium.py Fixed Show fixed Hide fixed
tests/adapters_t/sites_t/test_lancium.py Fixed Show fixed Hide fixed
tests/adapters_t/sites_t/test_lancium.py Fixed Show fixed Hide fixed
@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 2 alerts when merging dcdfbc8 into 7686dbb - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 2 alerts when merging ae9a4a1 into 7686dbb - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 2 alerts when merging b0af5e5 into 7686dbb - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 2 alerts when merging 8fa3c3d into 7686dbb - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@giffels giffels marked this pull request as ready for review November 17, 2022 14:30
@giffels giffels requested review from a team and removed request for a team November 17, 2022 14:31
@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 2 alerts when merging aa48d4b into c9db4cc - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

RHofsaess
RHofsaess previously approved these changes Nov 23, 2022
Copy link

@RHofsaess RHofsaess left a comment

Choose a reason for hiding this comment

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

Looks fine 👍

Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

Got some minor suggestions and comments. Please take a look at them, the PR is good to go otherwise.

docs/source/plugins/plugins.rst Show resolved Hide resolved
tardis/adapters/sites/lancium.py Outdated Show resolved Hide resolved
tardis/adapters/sites/lancium.py Outdated Show resolved Hide resolved
giffels and others added 2 commits November 23, 2022 16:54
Co-authored-by: Max Fischer <maxfischer2781@gmail.com>
@giffels
Copy link
Member Author

giffels commented Nov 23, 2022

Thanks a lot for the suggestions and comments.

@lgtm-com
Copy link

lgtm-com bot commented Nov 23, 2022

This pull request introduces 2 alerts when merging 077c621 into c9db4cc - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

Go for it!

Copy link

@RHofsaess RHofsaess left a comment

Choose a reason for hiding this comment

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

👍

@giffels giffels merged commit 7ddc223 into MatterMiners:master Nov 24, 2022
@giffels giffels deleted the add-lancium-adapter branch November 24, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants