Make variable definition more flexible for level 3 datasets#341
Make variable definition more flexible for level 3 datasets#341jonathanthiry wants to merge 7 commits into
Conversation
jonathanthiry
changed the title
ggVGc
left a comment
ggVGc
left a comment
There was a problem hiding this comment.
I did not look closely at the UI parts, and have not tested things locally, but left some implementation-specific comments.
Fundamentally I agree with using the end part of IRIs for identification of resources, as long as we have stable mapping between scala data types and IRI prefixes.
| case input: T => input | ||
| case _ => throw new Exception(s"Missing #$id element") |
There was a problem hiding this comment.
Why do we want to throw instead of returning Option as before? It makes more sense for me that the getElementById utility method is honest about the case of no result matching, and the exception is optionally thrown at the call-site where it makes sense, by using .get, as before.
There was a problem hiding this comment.
I understand, the reason is to get a better error message when the expected ID does not exist, which I can agree with.
What happens from user perspective when an exception is thrown, though? I see that we have a fail method here: https://github.com/ICOS-Carbon-Portal/meta/blob/jonathanthiry/l3-variables/uploadgui/src/main/scala/se/lu/nateko/cp/meta/upload/Utils.scala#L31
Does that serve a similar purpose?
There was a problem hiding this comment.
Yes, it is to get a better error message. We are showing the error in a banner at the top of the page. With this change you can know directly which id is missing. An id is most likely to be missing during development, and it is very useful to see directly which one is missing.
There was a problem hiding this comment.
I was also wondering about that, but your response makes perfect sense.
|
|
||
| private val plainLookup: Map[String, VarMeta] = varDefs.flatMap(_.plain).map(vm => vm.label -> vm).toMap | ||
| private val plainLookup: Map[String, VarMeta] = varDefs.flatMap(_.plain).map{vm => | ||
| vm.model.uri.toString.split('/').last -> vm |
There was a problem hiding this comment.
This pattern of extracting the ID from an IRI appears a few times in this PR, and in se.lu.nateko.cp.meta.api we have UriId for this purpose.
I think it would make sense to introduce a helper method for this purpose here too. I'm not sure where it would be most suitable, but maybe here in VarMetaLookup for now?
There was a problem hiding this comment.
Good point! I pushed another commit that makes use of UriId. I'll have to double-check that it works as expected.
|
|
||
| abstract class GenericTextInput[T](elemId: String, cb: () => Unit, init: Try[T])(parser: String => Try[T], serializer: T => String) { | ||
| private val input: TextInputElement = getElementById[html.Element](elemId).get.asInstanceOf[TextInputElement] | ||
| private val input: TextInputElement = getElementById[html.Element](elemId).asInstanceOf[TextInputElement] |
There was a problem hiding this comment.
If I understand correctly, this should be equivalent, since we are doing a runtime type check in a match in getElementById?
Or is this a case where we want to give an error if such a html.Element was not found, but crash if it was found but it was not a TextInputElement?
| private val input: TextInputElement = getElementById[html.Element](elemId).asInstanceOf[TextInputElement] | |
| private val input: TextInputElement = getElementById[TextInputElement](elemId) |
There was a problem hiding this comment.
I should have read better, this doesn't work. getElementById looks for a html.Element and TextInputElement doesn't extend html.Element but html.Object. There is probably a better way to write this, as it is confusing, but that would require a slightly large change.
Use TextInputElement directly Co-authored-by: Valter Sundström <valter.sundstrom@gmail.com>
Jonathan-Schenk
left a comment
Jonathan-Schenk
left a comment
There was a problem hiding this comment.
I do not have any major comment, just one minor suggestion about how variable names are displayed in the upload GUI. Feel free to take it into account or not.
| case input: T => input | ||
| case _ => throw new Exception(s"Missing #$id element") |
There was a problem hiding this comment.
I was also wondering about that, but your response makes perfect sense.
| } | ||
|
|
||
| private val varNameInput = new TextInput(s"varnameInput_$id", notifyUpdate, "variable name") | ||
| private val varNameInput = new Select[DatasetVar](s"varnameInput_$id", s => s"${s.label} (${s.title})", _.uri.toString, false, notifyUpdate) |
There was a problem hiding this comment.
Could we use a different format for the variable name in the input menu? Some variables contain parentheses in their label, resulting in things like fire (co2 fluxes) (fireC). It is for sure a rather minor, cosmetic thing, but I would suggest to use characters that are less likely to show up in the label and title of variables, like a vertical bar separator maybe.
There was a problem hiding this comment.
Good point. I went for brackets after testing a few different options.
3 participants
We would still use the label as the identifier to avoid having to migrate the current data. It should now be possible to define distinct variables that use the same title in the dataset. The upload form was updated to fetch a list of available variables, and use a select to pick the relevant ones.