refactor: remove redundant field in sub-routine definition

[schweikart]

The SubRoutineDefinition has been refactored to remove redundant information. This PR adjusts to the new SubRoutineDefinition data format.

Server-Side PR: ProvideQ/toolbox-server#40 (comment)

[Elscrux]

That wouldn't work for the SolverPicker from what I can tell. it is still referencing the problem type in here and you didn't chance that, right? https://github.com/ProvideQ/toolbox-web/blob/develop/src/components/solvers/SolverPicker.tsx#L123
And I don't see how we can do this without type and url. Well, we could get rid of the problemType in the solver picker props if needed as that's only used for displaying the type. But in any case that would need to be changed in this PR too.

I think this PR should be disregarded, as we need this in the subroutine definition. I already merged the server PR as I didn't notice you did it this way, by removing the whole parameter from the constructor, so I will add a fix to my pending PR. (I could make a separate PR too, but that would just mess things up even more)

[schweikart]

Sorry, this PR was supposed to be a draft first. I've changed it now to use the problemTypeId everywhere instead of type and url. Unfortunately, the server currently uses the ProblemType enum entry names instead of the ids, which I was unaware of.

I think this PR should be disregarded

I think the best solution is to change the server to use ProblemType#getId() for de/serialization so we can discard the ProblemType name on the client side.

I will add a fix to my pending PR. (I could make a separate PR too, but that would just mess things up even more)

Please don't mix this up, I don't see how putting that in another PR improves the situation.

[Elscrux]

When you pass problemTypeId into problemType, that is a bit confusing and will end up showing the id in the UI https://github.com/ProvideQ/toolbox-web/blob/develop/src/components/solvers/SolverPicker.tsx#L74
If we say this behavior is ok, that's fine then.

In that case, passing problemTypeId twice into SolverPicker shouldn't be a thing. Instead, there should be a boolean variable isSubroutine in SolverPickerProps instead of checking for undefined in the same code I linked above https://github.com/ProvideQ/toolbox-web/blob/develop/src/components/solvers/SolverPicker.tsx#L74

Edit: I still see a major problem when removing problemType, as requestedSubSolveRequests (also used on the server) is affected and wouldn't be a Map<ProblemType, SolveRequest<?>> anymore. It would be a lot worse as you'd need to have something like subRoutinePool.getSubRoutine("sat") in the eventual solvers. So I'd really just say keep the problem type and everything as it is. It's all working fine that way :)

[Elscrux]

I will add a fix to my pending PR. (I could make a separate PR too, but that would just mess things up even more)

Please don't mix this up, I don't see how putting that in another PR improves the situation.

I did this already, but I can remove it if we don't need the change I described in the previous comment.

[schweikart]

When you pass problemTypeId into problemType, that is a bit confusing and will end up showing the id in the UI https://github.com/ProvideQ/toolbox-web/blob/develop/src/components/solvers/SolverPicker.tsx#L74
If we say this behavior is ok, that's fine then.

But before, the enum constant name from our Java implementation was passed in there. I can't really see how MAX_CUT is any better than max-cut.

In that case, passing problemTypeId twice into SolverPicker shouldn't be a thing. Instead, there should be a boolean variable isSubroutine in SolverPickerProps instead of checking for undefined in the same code I linked above https://github.com/ProvideQ/toolbox-web/blob/develop/src/components/solvers/SolverPicker.tsx#L74

Makes sense, I can change that.

Edit: I still see a major problem when removing problemType, as requestedSubSolveRequests (also used on the server) is affected and wouldn't be a Map<ProblemType, SolveRequest<?>> anymore. It would be a lot worse as you'd need to have something like subRoutinePool.getSubRoutine("sat") in the eventual solvers. So I'd really just say keep the problem type and everything as it is. It's all working fine that way :)

The change is surprisingly pretty simple, I've done it in this PR: ProvideQ/toolbox-server#44

I did this already, but I can remove it if we don't need the change I described in the previous comment.

If we agree to switch to ProblemType#getId() for problem type identification, that would be nice 👍

[Elscrux]

But before, the enum constant name from our Java implementation was passed in there. I can't really see how MAX_CUT is any better than max-cut.

Yeah that's fine too

The change is surprisingly pretty simple, I've done it in this PR: ProvideQ/toolbox-server#44

That's a good workaround!

If we agree to switch to ProblemType#getId() for problem type identification, that would be nice 👍

I still think that the enum value itself is the better identifier for general usage (in the server logic), as we have a defined set of values which is way better than comparing string values. For communicating with the website, sure id is good, and avoiding having two different ways to identify a problem type is better anyway.
And there shouldn't be worries that it won't be unique as it will be unique anyway because it's used for the http calls.

[Elscrux]

@schweikart I did the changes myself to sync everything with the server repo.

[schweikart]

I still think that the enum value itself is the better identifier for general usage (in the server logic), as we have a defined set of values which is way better than comparing string values. For communicating with the website, sure id is good, and avoiding having two different ways to identify a problem type is better anyway.

Definitely!

I did the changes myself to sync everything with the server repo.

LGTM! I reviewed the changes and tested them with your server PR and it works as expected :)

Unfortunately I can't approve this PR as I'm its author, can you review, approve and merge please?