PackBuilder Fixup

[dahlbyk]

A few adjustments to #1183 that I arrived too late to suggest as feedback.

/cc @AMSadek @whoisj @nulltoken @ethomson @carlosmn @kevin-david

[whoisj]

Looks good.

Would still like to see PackBuilderOptions become a struct instead of a class.

[nulltoken]

@dahlbyk I'm not really sold on the fluent approach. Although it works in this context, I'd be grateful if we could move this to a separate PR and see if globally, from an PI standpoint this is a sustainable approach. (are there other types that could benefit from this? Are there types which semantic make that cumbersome...).

Besides this nitpick, I ❤️ your rebound.

[dahlbyk]

I added the fluency in imitation of TreeDefinition.Add(...).

Would still like to see PackBuilderOptions become a struct instead of a class.

It seems that most of our Options types are currently classes - should we prefer consistency over case-by-case optimization?

[AMSadek]

@whoisj

Would still like to see PackBuilderOptions become a struct instead of a class.

I vote for keeping it as a class. One more thing other than consistency is I don't want to allow default constructor here. So that I don't validate twice if the path has been set or not, and if it exists or not.

@dahlbyk Thanks for the feedback! ...
I'm not sure how useful the chaining support you added will be, however I don't object to it.
Other than that, the changes look fine to me :)

[nulltoken]

I added the fluency in imitation of TreeDefinition.Add(...).

@dahlbyk Indeed. However, if that makes sense to you, I'd like to ensure that the PackBuilder is stable from the API perspective before moving on with some fluency.

In #1183 (comment), @AMSadek was raising an additional need that isn't tackled yet. I'd prefer postpone the chainable method approach for this type until we've defined how the API should handle the split in multiple packs. Deal?

@AMSadek Do you still need to perform this kind of splitting?

[dahlbyk]

Dropped fluency (still available at 34779a2 until GitHub gc).

[nulltoken]

Dropped fluency (still available at 34779a2 until GitHub gc).

Any chance you could push it to a branch?

[nulltoken]

@dahlbyk ❤️

[dahlbyk]

Any chance you could push it to a branch?

Done and PR'd

[AMSadek]

@dahlbyk thanks for the fixes :)!

@nulltoken

@AMSadek Do you still need to perform this kind of splitting?

Yup, kindly have a look on my recent PR #1216 :)