Design standards question

[mapitman]

In most of the classes that have a Blocks property, it is an IList<IMessageBlock>, however in the View class it is an array. Is this by design or can we change it to a list?

Slack.NetStandard/Slack.NetStandard/Messages/Message.cs

Line 18 in 4063c5c

public IList<IMessageBlock> Blocks { get; set; } = new List<IMessageBlock>();

Slack.NetStandard/Slack.NetStandard/Objects/View.cs

Line 36 in 4063c5c

public IMessageBlock[] Blocks { get; set; }

[stoiveyp]

Hi @mapitman - this has been on my "maybe" list for quite some time.

Context

Firstly - it's definitely not on purpose. It's fair to say that I didn't start this library expecting to build full support for everything - I was just working on some Slack slash commands and wanted it to be a little easier. It....grew 😄

What this meant was that the objects at this stage are still based on the order I built them - when I built messages I was sending them, so they were a list. Views were something I expected to receive in events - so they were an array. Then I looked at modals and the Web API support was written (it came last) and now I was building Views - but the package had already gone through some big changes so I left them as it was.

Future Design

Okay - so your actual question. Can we change it? Yes we can - v4.0 (it would be a breaking change - so a new major version) could update the models. So when I've thought about this before the rules would be

• If it's only ever to be received - it's an array
• if it can be sent - it's a list

The only other thing is that I don't want to build objects that rely on custom contracts to send data out - so every list will need a ShouldSerializeXXX method or a converter (Slack API tends to error on empty lists being sent - they need to be ignored if they're empty)

Does that sound like it would work?

[mapitman]

Yes, sounds great! I wanted to ask before creating an issue or a pull request. Is 4.0 something you're planning soon or is there some time? If there's some time, I'll see if I can get some time to submit a PR.

[stoiveyp]

Sorry for not replying sooner - my time on a laptop has been a bit hectic in the last week.

In order to ensure this is an easy set of guidelines to follow, I have published a branch and started adding some new Roslyn Analyzers to the project in order to ensure these are guidelines we can stick to.

I've got two outstanding tasks before this can go into a V4 beta

• Review all the existing array objects and update to IList<T> where appropriate
• Create a contributing.md explain the analyzers and why they're there.

I'm hoping to get this work done this week so I can release 4.0.0-beta1 by the weekend 🤞

[stoiveyp]

I pushed V4 this morning - there were a lot more arrays in the request objects than I had realised.

I've also added to the Analyzers so that future changes are raised as a warning

So now the project compiles checking for the following added rules

1. If a property is an array - a warning is raised unless it matches one of these conditions

• it's in the Events API namespace
• The class name it's in has Response in it
• It has an AcceptedArray attribute added to it (a new attribute I added to the project)
• The class it's contained in has an AcceptedArray attribute added to it

There is a code fix added to the warning to convert the property to an IList<T> if none of these apply

2. For IList<T> properties

• The list must be initialised
• There must be a ShouldSerialize[propertyName] method in the containing class to ensure it's not added if empty

Code fixes are available for both these warnings too (they were well worth me writing them - made updating the library so much easier 😁 )

I could have written a contract resolver to handle the IList properties instead of lots of ShouldSerialize methods, but then anywhere there's serialization outside of the library you have to remember to add the contract and that's a no from me.