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

feat: migrate issues GET APIs to GithubSDK #145

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JeffreytheCoder
Copy link
Contributor

@JeffreytheCoder JeffreytheCoder commented Apr 13, 2024

Issue(s)

Fixes a part of #143

Acceptance Criteria fulfillment

Completed 6 migration tasks related to issues GET APIs

  • 13. Migrate getIssueTemplates method
  • 14. Migrate getIssueTemplateCode method
  • 20. Migrate getUserAssignedIssues method
  • 21. Migrate getIssueData method
  • 24. Migrate getIssuesComments method
  • 27. Migrate getRepositoryIssues method

@VipinDevelops
Copy link
Contributor

Hey @JeffreytheCoder great work with the migration I would suggest you attach a Video to show case that the Features are working as expected with new changes.

@JeffreytheCoder
Copy link
Contributor Author

Thanks @VipinDevelops! I saw you created a GitHubApi instance for calling getBasicUserInfo in #144 .

let BaseHost = await read.getEnvironmentReader().getSettings().getValueById(AppSettingsEnum.BaseHostID);
let BaseApiHost = await read.getEnvironmentReader().getSettings().getValueById(AppSettingsEnum.BaseApiHostID);
const gitHubApiClient = new GitHubApi(
http,
access_token,
BaseHost,
BaseApiHost
);
userInfo = await gitHubApiClient.getBasicUserInfo();

But in this way, everytime we need to call an API, we need to create a instance and fetch properties like access_token and BaseHost.

I'm thinking of creating a GitHubApi singleton on app setup. On creation, the singleton will fetch and store the properties. All app actions can call APIs using the singleton. What do you think?

@VipinDevelops
Copy link
Contributor

Hey @JeffreytheCoder Yesterday, @samad-yar-khan Also shared about this and suggested a new way to improve this with a nice reference,

Your approach is good, but let me first research the method Samad shared and then I can make a new PR that fixes this issue.

@samad-yar-khan
Copy link
Collaborator

@JeffreytheCoder @VipinDevelops I was thinking along the same lines as @JeffreytheCoder. We should think about two things while approaching the design for this class

  • It should be easy to use.
  • It should be easy to extend/update.
  • Is there a performance degradation due to fetching of token and URL each time this is used ?
  • Can the user update the base url/token in setting and is the code able to use the updated configuration.

@JeffreytheCoder
Copy link
Contributor Author

@samad-yar-khan Thanks for the insights! Like my comment above, I think having token & URL stored in a singleton solves the issue of fetching themevery time. Whenever the user updates token & URL in the settings, we update the singleton too. What do you think?

@VipinDevelops
Copy link
Contributor

VipinDevelops commented May 6, 2024

I agree with @samad-yar-khan. @JeffreytheCoder, can you please implement it that way in this PR? Then, Samad can review it, and we can move forward from there and also update the few places where this new class is being used.

@JeffreytheCoder
Copy link
Contributor Author

Implemented the singleton approach and applied to all migrated methods in this PR and in #144.

Behavior same as before:
singleton

@JeffreytheCoder
Copy link
Contributor Author

The singleton gets api host from environment and generates access token when initialized. We'll need to update these when user update settings / login / logout in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants