Skip to content

C#: Need a configuration setting to change the type for arrays #1338

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

Closed
artem-dudarev opened this issue Oct 5, 2015 · 9 comments
Closed

Comments

@artem-dudarev
Copy link
Contributor

Currently arrays are generated as List<T>. I'd like to configure the generator to get real arrays T[].

@wing328
Copy link
Contributor

wing328 commented Oct 6, 2015

@artem-dudarev do you mind sharing more about the requirement to use T[] instead of List<T>? Is that a must or more of a personal preference?

Before I started working on the C# generator, it was already using List instead of [] so I just keep it that way (both are fine with me).

@wing328 wing328 added this to the Future milestone Oct 6, 2015
@wing328 wing328 added the P3 label Oct 6, 2015
@artem-dudarev
Copy link
Contributor Author

This is not a must. I just think the property or method definition is shorter and more readable with array then with list.
Thats why I ask for configuration option.

@wing328
Copy link
Contributor

wing328 commented Oct 7, 2015

@artem-dudarev thanks for the suggestion. We'll leverage the help from the community to implement this in the future.

@jimschubert
Copy link
Contributor

Arrays aren't a direct replacement for List<T>. Specifically because initializing an array requires the array size and resizing an array is not cheap. Lists are a lot more efficient in practice.

If anything, I think IEnumerable<T> as exposed types and List<T> as internal types would be preferable.

@wing328
Copy link
Contributor

wing328 commented Jan 23, 2016

@jimschubert thanks for the suggestion. May I know if you've cycle to contribute the enhancement?

@jimschubert
Copy link
Contributor

I could do this work, but it would be a breaking change. For example,

List<Pet> FindPetsByStatus (List<string> status = null);

would become

IEnumerable<Pet> FindPetsByStatus(IList<string> status = null);

This would break code previously generated against List<Pet> by:

  • Removing the indexer (e.g. pets[1])
  • Removing ability to modify the collection
  • Requiring explicit type usage to be changed (e.g. List<Pets> pets = client.FindPetsByStatus())

The benefit of using IEnumerable<T> would be that you're free to change any internal implementation without breaking consumers in the future.

The Framework Guidelines specifically say to not return List<T> from a public API, which I'd consider generated code. The guidelines suggest using Collection<T>, although when implementing APIs I try to stick with ICollection<T>. They've somewhat hidden my IEnumerable<T> suggestion within that collection suggestion:

In cases where you are sure that the only scenario you will ever want to support is forward-only iteration, you can simply use IEnumerable<T>.

I'd need to look at the code a little more closely to see how much effort it would be.

@jimschubert
Copy link
Contributor

I created the linked pull request to demonstrate what this change would mean for consumers (see the updated tests). I chose ICollection<T> over IEnumerable<T> because ICollection<T> allows for modification.

@wing328
Copy link
Contributor

wing328 commented Jan 25, 2016

@jimschubert thanks for the contribution. PR merged into master.

@wing328 wing328 closed this as completed Jan 25, 2016
@alhalama
Copy link

@jimschubert @wing328 While there are reasons not to expose List<T>, I don't believe they apply to IList<T> and that would have been less of a breaking change as that would still allow indexing into the collection.

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

No branches or pull requests

4 participants