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

#982 unable to find path for getArchive api #1036

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Conversation

DoubleDPro
Copy link
Contributor

Added "path" parameter to methods

Fixes #982

@jmini
Copy link
Collaborator

jmini commented Oct 2, 2023

Why did you try to ByPathAndSha to each of the methods? Was it mandatory because of a naming clash?

@DoubleDPro
Copy link
Contributor Author

Was it mandatory because of a naming clash?

Yes. There is a naming clash with deprecated method so I decided to rename new methods. If you have a better solution, I will correct my PR.

@jmini
Copy link
Collaborator

jmini commented Nov 16, 2023

Sorry for the delay.

I think it would be cleaner to add a RepositoryArchiveParams providing all the possible attributes with a builder pattern withSha(String), withPath(String), withFormat(ArchiveFormat) and so on.

And then we would have a method:

  • getRepositoryArchive(Object projectIdOrPath, RepositoryArchiveParams params) returning an InputStream
  • getRepositoryArchive(Object projectIdOrPath, RepositoryArchiveParams params, File directory) returning a File

The existing methods can delegate to the new one.

This way we have the maximum flexibility of providing the parameters we would like. Methods overloading is no longer an issue.

This is a pattern we already have at multiple places.

@DoubleDPro what do you think?

@DoubleDPro
Copy link
Contributor Author

@jmini it looks good to me. I will correct it.

@DoubleDPro
Copy link
Contributor Author

@jmini I corrected my PR. Can you check it?

Copy link
Collaborator

@jmini jmini left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I will perform some testing and then merge

@jmini jmini merged commit 4440328 into gitlab4j:main Dec 22, 2023
1 check 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.

unable to find path for getArchive api
2 participants