Skip to content

Implements parameters for RepositoryClient.GetArchive#659

Open
steve85 wants to merge 8 commits into
ubisoft:mainfrom
steve85:implements-getarchive-params
Open

Implements parameters for RepositoryClient.GetArchive#659
steve85 wants to merge 8 commits into
ubisoft:mainfrom
steve85:implements-getarchive-params

Conversation

@steve85

@steve85 steve85 commented Apr 11, 2024

Copy link
Copy Markdown
Contributor

Implements the format, path and sha options for RepositoryClient.GetArchive().

https://docs.gitlab.com/ee/api/repositories.html#get-file-archive

Allows us to retrieve the archive for a particular SHA/ref and specify the required download format.

@steve85 steve85 requested a review from a team as a code owner April 11, 2024 04:56
@steve85 steve85 requested review from louis-z and removed request for a team April 11, 2024 04:56
Comment thread NGitLab/IRepositoryClient.cs Outdated
Comment thread NGitLab/Models/FileArchiveFormat.cs
Comment thread NGitLab/Extensions/TypeExtensions.cs Outdated
@steve85 steve85 requested a review from louis-z April 12, 2024 03:04
@steve85 steve85 changed the title Implements SHA and Format parameters for RepositoryClient.GetArchive Implements parameters for RepositoryClient.GetArchive Apr 12, 2024
@steve85 steve85 force-pushed the implements-getarchive-params branch from 93889b4 to 359668b Compare April 15, 2024 02:14
@steve85

steve85 commented Apr 18, 2024

Copy link
Copy Markdown
Contributor Author

@louis-z can this PR now be merged or are there further outstanding changes you'd like me to make?

@louis-z

louis-z commented Jun 15, 2026

Copy link
Copy Markdown
Member

@steve85, this PR is stale. Is there a plan to resurrect it any time soon? If not, we'll consider closing it.

@steve85 steve85 force-pushed the implements-getarchive-params branch from 359668b to 309ce90 Compare June 17, 2026 05:51
@steve85

steve85 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Hi @louis-z - I was originally waiting on a follow up review from you, hence the lack of activity on this.

I have rebased the branch and made some changes to fix some minor things. Please take a look and let me know if there are any further issues that need addressing.

Comment thread NGitLab.Tests/Impl/UtilsTests.cs Outdated
var actual = NGitLab.Impl.Utils.AppendSegmentToUrl<string>(url, value: null, includeSegmentSeparator: false);

// Assert
Assert.That(expected, Is.EqualTo(actual));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please invert actual and expected, in conformance with the NUnit Constraint Model's API + do the same where applicable.

@louis-z

louis-z commented Jun 22, 2026

Copy link
Copy Markdown
Member

Hi @louis-z - I was originally waiting on a follow up review from you, hence the lack of activity on this.

I have rebased the branch and made some changes to fix some minor things. Please take a look and let me know if there are any further issues that need addressing.

I wrote a comment back in 2024 but forgot to submit it... 🙄 Sorry about that.

It's this one.

Comment thread NGitLab/Models/FileArchiveFormat.cs Outdated
[EnumMember(Value = ".bz2")]
Bz2,

[EnumMember(Value = ".gz")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the .gz format truly supported? I don't see it mentioned in the GitLab doc, but then again it may be outdated...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's been so long since I wrote the original code I'm not sure what I was referencing when I added .gz. I have removed it.

Comment thread NGitLab/Impl/Utils.cs Outdated
if (value is null)
return url;

// Don't allow segments to a url which already has parameters present

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Don't allow segments to a url which already has parameters present
// Don't allow appending segments to a url which already has parameters present

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread NGitLab/Impl/Utils.cs Outdated
return url + parameter;
}

private static string GetEnumMemberValue<T>(string valueString)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you really need this method? I have a feeling you could use ToValueString<T>(T value) instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, this method wasn't needed. I've removed it.

Comment thread NGitLab.Tests/Impl/UtilsTests.cs Outdated
Assert.That(url, Is.EqualTo($"{basePath}?event_action={expectedQueryParamValue}"));
}

[TestCase]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use [Test] here instead and for any other non-parameterized test method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@steve85 steve85 force-pushed the implements-getarchive-params branch from adb18c4 to be79730 Compare June 23, 2026 11:06
@steve85 steve85 requested a review from louis-z June 23, 2026 12:04
@steve85 steve85 force-pushed the implements-getarchive-params branch from be79730 to 301f853 Compare June 23, 2026 23:26
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.

2 participants