search

StoutLogic / acf-builder

An Advanced Custom Field Configuration Builder
GNU General Public License v2.0
792 stars 61 forks source link

#166 Replace NamedBuilder interface with FieldBuilder #167

Closed michaelw85 closed 1 year ago

michaelw85 commented 1 year ago

Removed interface NamedBuilder and replaced typings with FieldBuilder to improve intellisense.

Before: image

After: image

stevep commented 1 year ago

I pulled the PR down and tested the auto complete and like your screenshots it's great!

The only thing I'm hung up on is the total removal of the NamedBuilder interface. I'm not sure it is necessary, but also I don't remember the interface's purpose!

Allow me a couple days to think about it before merging this PR.

michaelw85 commented 1 year ago

I pulled the PR down and tested the auto complete and like your screenshots it's great!

The only thing I'm hung up on is the total removal of the NamedBuilder interface. I'm not sure it is necessary, but also I don't remember the interface's purpose!

Allow me a couple days to think about it before merging this PR.

If you want to add the interface just to be sure I could revert it and add a depreciation warning. For it's purpose I can only guess but maybe in the NamespaceFieldkey which I changed to a builder type since it was not using getName.

stevep commented 1 year ago

Thanks @michaelw85

I ended up bringing back the NamedBuilder interface because the NamespaceFieldKey transform technically calls getName on the builder which doesn't exist on the plain Builder

If you pull the latest, does the autocomplete still work as you expect?

If so I'll go ahead and merge.

michaelw85 commented 1 year ago

@stevep Intellisense still works. I missed the getName call because it's called on the builder. So technically I think it would make more sense to add it to the builder interface. Since the FieldBuilder and FieldsBuilder both inherit the ParentDelegationBuilder which in turn uses the Builder interface.

But I think I would merge this PR in the current state since it does solve the issue I raised. Refactoring could be split into a separate enhancement/refactor ticket which could be resolved on its own. (I'm willing to do the changes).