Remove hardcoded fields #170
Conversation
src/api/SubjectsApi.ts
Outdated
| case FieldType.Link: | ||
| acc[ fieldName ] = newLinkField( | ||
| rawValue, | ||
| ...( parsedValue.split( '||' ) as [ string, string ] ) |
There was a problem hiding this comment.
@psrpinto The only way I could think of supplying link's parsed value in a single string was to use a separator like ||. Let me know if you have ideas around handling this better.
There was a problem hiding this comment.
I'm not sure what the correct solution here would be. For the moment, I think we could postpone this decision by not handling link here (removing the case for it) as there will be no field coming from the API of type link yet. At the moment, LinkField is only used in the UI (to parse navigation), but it's not yet sent to the server (as there is no need to store it yet).
Once we add link as a supported type of field in the schema, we would come back to this and figure out what to do. Would that make sense to you?
There was a problem hiding this comment.
Sure, I will comment out this much code rather than just deleting it.
There was a problem hiding this comment.
I'm not sure I see the value in keeping this code around, as it's a simple call to newLinkField(), so I think we'd be able to write the code again easily. I also have concerns about the specific implementation (using ||), as to me that is a sign that there's something off in the way we're expressing the data model. I think if we need to resort to this sort of "hack", something must be off in the data model.
That's why I was suggesting to just remove it, because I think that properly supporting LinkField (and other fields that are not just a string) will require re-evaluating the approach we're currently using for sending fields across the API.
If you think there is value in keeping this code commented out, I'm ok with it, but I generally don't see the value in keeping commented out code in repos.
There was a problem hiding this comment.
The value that I see in keeping this commented code is to serve as a pointer to come back to this exact discussion of what we discussed, what we were trying to do and why it didn't work. Other than that, I don't have any reasons to keep it around and if you prefer to keep commented code out, I will go ahead and remove it.
There was a problem hiding this comment.
keeping this commented code is to serve as a pointer to come back to this exact discussion
I didn't understand your intention initially, I'm fine with this. In that case, would it make sense to add a comment in the code with a link to this discussion?
There was a problem hiding this comment.
Could have explicitly linked the PR instead indeed, but merged it already now.
src/api/SubjectsApi.ts
Outdated
| case FieldType.Link: | ||
| acc[ fieldName ] = newLinkField( | ||
| rawValue, | ||
| ...( parsedValue.split( '||' ) as [ string, string ] ) |
There was a problem hiding this comment.
I'm not sure what the correct solution here would be. For the moment, I think we could postpone this decision by not handling link here (removing the case for it) as there will be no field coming from the API of type link yet. At the moment, LinkField is only used in the UI (to parse navigation), but it's not yet sent to the server (as there is no need to store it yet).
Once we add link as a supported type of field in the schema, we would come back to this and figure out what to do. Would that make sense to you?
closes #161