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 GitHub Adapter #1

Merged
merged 151 commits into from
Aug 9, 2023
Merged

Add GitHub Adapter #1

merged 151 commits into from
Aug 9, 2023

Conversation

vermakhushboo
Copy link
Member

No description provided.

@vermakhushboo vermakhushboo marked this pull request as draft March 1, 2023 07:28
@vermakhushboo vermakhushboo marked this pull request as ready for review April 5, 2023 14:57
docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Member

@eldadfux eldadfux left a comment

Choose a reason for hiding this comment

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

  • Order of all operations needs to be organized. All operations from the same group (repository, branch, comment) have to be grouped together one after the other. Order should be consistent createX -> listX -> getX -> updateX -> deleteX
  • validateWebhookEvent can be a validator, we use it only once, but instead of having a very custom and non-generic validator, I would check if we can just use existing and specific validates in Appwrite to just check for the primitive data forms we’re expecting to get and avoid the all object structure. Going lower level and keeping it simple.
  • isGitFlow can be getType and we can maintain a few constants like VCS::TYPE_GIT and VCS::TYPE_SVN and return them. Makes more since than addressing each type for future support.
  • Don’t directly access array prop like return $response['body']; without null safety check, it should be accessed like return $response['body'] ?? ‘’; where the default value should be of the same type you expect. In many places it would also be wise to check for the value existence and throw an exception if not found.
  • What is the result of downloadRepositoryZip downloadRepositoryTar? Is it the file contents? If yes, what happens when the file is extremely large? This could block our entire application memory and should be handled in chunks.
  • If we initialize the GitHub adapter in many places anyway, I don’t see the point in using it with the dependency injection. For that, initialiseVariables can be deleted and combined with the __construct method.
  • I don’t find any usage of generateAccessToken, can we delete it?
  • getTotalReposCount can be named getRepositoriesTotalCount, This way we keep everything consistent and when we look for a specific attribute in a response it also the last part in the method name. Our naming convention should lean to be first programatic and then human readable, in most cases both will work very well together.
  • I’m not sure what we should do with generateCloneCommand this is the only method that is interacting with the Git CLI instead of the API. Let’s move it to the Git adapter and add a comment to our backlog to see how we can redesign it to make sense in this context or another one.
  • If we return an array, let’s use list as prefix. getRepositoryLanguages can be listRepositoryLanguages.
  • forkRepository can become createFork.
  • Can we rename listRepositoriesForVCSApp to listRepositories?

@eldadfux
Copy link
Member

eldadfux commented Aug 8, 2023

Also, @vermakhushboo I think 99% of the methods you have in the GitHub adapter should also be in the abstract adapter.

src/VCS/Adapter/Git.php Outdated Show resolved Hide resolved
src/VCS/Adapter/Git/GitHub.php Outdated Show resolved Hide resolved
src/VCS/Adapter.php Outdated Show resolved Hide resolved
src/VCS/Adapter.php Outdated Show resolved Hide resolved
Copy link
Contributor

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Another general comment that we can tackle in the next version is

  • initialiseVariables should be like a factory static method called GitHub::initialise() or GitHub::init()

@christyjacob4 christyjacob4 merged commit fc9c38a into main Aug 9, 2023
3 checks passed
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.

5 participants