Skip to content
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

feat: add interface definitions for procedure stuff #6488

Merged
merged 8 commits into from
Oct 13, 2022

Conversation

BeksOmega
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

N/A

Proposed Changes

Adds definitions for interfaces related to the shared procedures project.

Reason for Changes

Interfaces are useful for documenting intentions, and they also keep the code flexible.

Test Coverage

N/A

Documentation

N/A

Additional Information

N/A

@BeksOmega BeksOmega requested a review from a team as a code owner October 5, 2022 20:47
@BeksOmega BeksOmega requested a review from gonfunko October 5, 2022 20:47
@cpcallen cpcallen self-requested a review October 6, 2022 10:56
@cpcallen cpcallen assigned cpcallen and unassigned gonfunko Oct 6, 2022
@cpcallen cpcallen removed the request for review from gonfunko October 6, 2022 10:57
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's clear from the comments below that I skimmed a bit too much when reading your design doc previously. I am not asserting that the design should necessarily be changed, but evidently I do think that it could use a bit more scrutiny (and in places justification).

core/interfaces/i_parameter_model.ts Outdated Show resolved Hide resolved
core/interfaces/i_parameter_model.ts Outdated Show resolved Hide resolved
core/interfaces/i_procedure_map.ts Outdated Show resolved Hide resolved
core/interfaces/i_parameter_model.ts Show resolved Hide resolved
core/interfaces/i_procedure_model.ts Outdated Show resolved Hide resolved
@cpcallen
Copy link
Contributor

cpcallen commented Oct 6, 2022

Additionally, having reviewed #6489, I wonder about the return types of many of the IProcedureModel methods. It seems evident that you want them to form a fluent interface, but I'm sceptical about the utility of this:

  • First of all I should declare up front that I have a general aversion to that style of interface. There are some situations where it seems justified, but it adds considerable potential for confusion, especially if not all the methods on the class return this.
    • E.g. at what point in the long chain of obj.x().y().z()… did we stop dealing with the original obj? How would one guess that it was at .insertParameter()??
  • Since it's easier to add a return value than take one away, I'd always err on the side of returning void until there is a compelling case to do otherwise.
    • One might find that one wants to return something other than this, after all, and then one is in real trouble!
  • I think it's better to write the non-fluent client code and only consider switching to a fluent interface. (Maybe you already did this and have the result squirrelled away in another branch?)

@BeksOmega
Copy link
Collaborator Author

  • First of all I should declare up front that I have a general aversion to that style of interface. There are some situations where it seems justified, but it adds considerable potential for confusion, especially if not all the methods on the class return this.

Cool cool, good to have context.

I understand how this could cause confusion, and I am amenable to changing the return type of insertParameter. However, in general, I think that a fluent interface is nice:

workspace.getProcedureMap()
    .addProcedure(new ObservableProcedureModel(workspace))
    .insertParameter(new ObservableParameterModel(workspace, new VariableModel(workspace)), 0)

as opposed to:

const proc = new ObservableProcedureModel(workspace);
proc.insertParameter(new ObservableParameterModel(workspace, new VariableModel(workspace)), 0);
workspace.getProcedureMap().addProcedure(proc);

I (and I think most people?) find that a fluent API is more readable, because each action leads logically to the next.

  • E.g. at what point in the long chain of obj.x().y().z()… did we stop dealing with the original obj? How would one guess that it was at .insertParameter()??

Like I said, I am amenable to changing .insertParameter but I'm not concerned about people "guessing" where the return type changes. IDEs will tell you the return types of functions, and will provide intellisense suggestions based on that.

This works similarly for our .appendInput and .appendField methods on Block.

  • Since it's easier to add a return value than take one away, I'd always err on the side of returning void until there is a compelling case to do otherwise.
    • One might find that one wants to return something other than this, after all, and then one is in real trouble!

In generally I appreciate being extremely flexible/cautious with the API. But in this case, I don't think that's a change that's likely to happen. E.g. I can't see a situation where I'd want to return something different from addProcedure or setReturnType.

  • I think it's better to write the non-fluent client code and only consider switching to a fluent interface. (Maybe you already did this and have the result squirrelled away in another branch?)

No, that's what the design doc is for :P

core/interfaces/i_parameter_model.ts Outdated Show resolved Hide resolved
core/interfaces/i_procedure_map.ts Outdated Show resolved Hide resolved
@BeksOmega BeksOmega force-pushed the feat/procedure-map-interfaces branch from 5ba7faa to dab40a4 Compare October 13, 2022 16:53
@BeksOmega BeksOmega deleted the feat/procedure-map-interfaces branch May 14, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants