Added DataLakeDirectoryClient.GetPaths()#16089
Conversation
seanmcc-msft
requested review from
KrzysztofCwalina,
amishra-dev,
amnguye,
kasobol-msft and
tg-msft
as code owners
ghost
added
the
| bool recursive = default, | ||
| bool userPrincipalName = default, |
There was a problem hiding this comment.
should this be options bag?
There was a problem hiding this comment.
I think this is the way it is for DataLakeFileSystemClient.GetPaths() too. I do like the idea of an options bag though, but if we do it here, we should go back and do it for the FileSystemClient too and hide the old one.
There was a problem hiding this comment.
I was also thinking about this when I did the implementation, I wanted to see what y'all thought. I'll switch both to options bags.
There was a problem hiding this comment.
Do we think this will add a lot of parameters in the future? You currently have 2 and the informal guideline is roughly 6.
There was a problem hiding this comment.
It's possible it could grow, but I don't know that there are any specific plans. I added this based on @kasobol-msft and @amnguye's comments.
There was a problem hiding this comment.
I'd follow this if there's no formal guidance. Let's not forget cancellationToken when we count parameters.
There was a problem hiding this comment.
That link offers a highly opinionated view that clashes with the rest of our (different highly opinionated 😄) style. In general for questions like this we should get them clarified in the guidelines rather than seek elsewhere so Storage libraries look and feel the same as other Azure services. I checked https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-parameters and it does now list 6 parameters:
Simple methods are methods that take up to six parameters, with most of the parameters being simple BCL primitives. ... Simple methods should follow standard .NET Design Guidelines for parameter list and overload design.
Unless we expect this to more than double, I'd suggest using parameters for now.
There was a problem hiding this comment.
Surprisingly the https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-parameters gives an example of "simple method" that has 3 parameters (2 BCL + cancellation token). In general I think 6 is too much and I think the sdk guide should trim that to 3 (6 parameter method doesn't look good imo).
Anyway in this case since we 2 params + token so let's follow the guideline then and go back to original design.
| IList<PathItem> paths = await response.ToListAsync(); | ||
|
|
||
| // Assert | ||
| Assert.AreEqual(3, paths.Count); |
There was a problem hiding this comment.
I'd assert path names as well in these tests to make sure we're getting the right one.
seanmcc-msft
changed the title
|
/azp run net - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@tg-msft, @kasobol-msft, I removed the options bags. Can we merge? |
Yes please. |
|
net - storage - ci |
seanmcc-msft
deleted the
feature/storage/dataLakeDirectoryClientGetPaths
branch
4 participants
Resolves #13216