Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add amt website client #136

Conversation

ChristopherSpelt
Copy link
Contributor

@ChristopherSpelt ChristopherSpelt commented Aug 13, 2024

Add AMT website client to fetch instruments

Implementation notes:

  1. Needed to change the existing method signature of list_content of the original GitHub client so that all implementations of the method list_content abstract client have the same signature.
  2. All clients are in 1 single file, to avoid 4 different small files. Ideally one would want the abstract class Client and get_client to be one 1 file, and GitHubClient in another file and GitHubPagesClient in yet another file, but this would result in circular imports.

Resolves #126

Checklist

Please check all the boxes that apply to this pull request using "x":

  • I have tested the changes locally and verified that they work as expected.
  • I have followed the project's coding conventions and style guidelines.
  • I have rebased my branch onto the latest commit of the main branch.
  • I have squashed or reorganized my commits into logical units.
  • I have read, understood and agree to the Developer Certificate of Origin, which this project utilizes.

@ChristopherSpelt ChristopherSpelt requested a review from a team as a code owner August 13, 2024 08:57
@ChristopherSpelt ChristopherSpelt linked an issue Aug 13, 2024 that may be closed by this pull request
berrydenhartog
berrydenhartog previously approved these changes Aug 13, 2024
Copy link
Member

@berrydenhartog berrydenhartog left a comment

Choose a reason for hiding this comment

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

LGTM

: Outdated Show resolved Hide resolved
@ChristopherSpelt ChristopherSpelt force-pushed the 126-use-website-to-get-instruments-in-amt-instead-of-github branch from 9be8ba4 to f07ed4d Compare August 13, 2024 10:06
@ChristopherSpelt ChristopherSpelt force-pushed the 126-use-website-to-get-instruments-in-amt-instead-of-github branch from f07ed4d to f19ca73 Compare August 13, 2024 10:09
Copy link

@ChristopherSpelt ChristopherSpelt merged commit 3f68457 into main Aug 13, 2024
15 checks passed
@ChristopherSpelt ChristopherSpelt deleted the 126-use-website-to-get-instruments-in-amt-instead-of-github branch August 13, 2024 10:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use website to get instruments in AMT instead of github
3 participants