Full layout metadata, layout named valueList, dateformats

[LeJeko]

The current version of fmrest limits the data returned by the get_layout function to fieldMetaData, omitting PortalMetaData and valueLists.
=> return response.get('fieldMetaData', None)

As I also needed access to the valueLists for my own development, I had to modify fmrest directly.

• To ensure backward compatibility, I've added a get_layout_metadata function that returns ALL layout metadata.
• Based on this, added a get_layout_valueList(name) function to retrieve a named valueList from a layout, formatted for direct use in a form's SELECT.
• Added optional dateformats parameter to API requests.

[LeJeko]

Missing name parameter in example: fms.get_layout_valueList('myListValue', 'myOptionalLayoutName')

[davidhamann]

Thanks for the PR. I left some comments in the code for suggestions/questions/remarks. Let me know what you think.

[davidhamann]

I'd prefer API_DATE_FORMATS here to be in line with the snake_case formatting.

Also, can't we use integers here? The API seems to be OK with sending the date format as integer (e.g. in the find request's POST data). In the past some parameters caused issues when sent as integer (for example offset and limit weirdly always needed to be strings to be accepted). But I think they fixed this in the DAPI before introducing the dateformats parameter.

[LeJeko]

OK for snake_case formatting.
I choose str to be in line with API format specified in apidoc:
https://you_host/fmi/data/apidoc/#tag/records/operation/getRecords

QUERY PARAMETERS
dateformats
string
The date format. Use 0 for US, 1 for file locale, or 2 for ISO8601. If not specified, the default value is 0.

[davidhamann]

I would suggest to call the parameter date_format (singular) to be in line with the other naming, even though it later gets translated to dateformats (plural, one word) for the API. This would then need to be consistent for all the other methods as well.

Also, it would be nice if we could pass keywords instead of the numbers; maybe us, file, iso-8601? Not passing a value or passing the default None would default to whatever the default of the DAPI is.

[LeJeko]

Ok to use date_format parameter if you prefer. I just use same name to avoid typing error ;-)

If we use keywords, we also need to had logic to convert them.
Is this code a way to to that ?

API_DATE_FORMATS = [('us', '0'), ('file', '1'), ('iso-8601', '2')]

date_format = 'file'

matching_format = next((format[1] for format in API_DATE_FORMATS if format[0] == date_format), None)
if matching_format:
    params['dateformats'] = matching_format

[davidhamann]

Since the default format could change for the DAPI in a different API version, I wonder if it might be better to not specify any date format when the user doesn't explicitly passes one to this method. Otherwise, we might hardcode the 0 = US value, even if a v3 of the DAPI might switch to a default value of iso-8601.

What do you think?

Preferably, the default for the library would be the ISO format, but I guess that's too late and wouldn't be backward compatible :-)

[LeJeko]

I also think it would make more sense to have the ISO format by default, but Claris has chosen the 0-US format.
Is it a good idea to change the default behavior of FMS?
And it may indeed be a little late to change the default value for those already using the library.

[davidhamann]

You're right that it's not optimal that get_layout only returns the fieldMetaData. However, instead of creating a new function that does the same except for its return value, I would suggest we give get_layout a new parameter to control what is returned (and default to the current case to remain backward compatible). For example: metadata=fields in the function signature, and then have options for portals, value_lists, or all.

[LeJeko]

Like this ?

def get_layout(self, layout: Optional[str] = None, metadata: Optional[str] = None) -> Dict:
        """Fetches layout metadata and returns Dict instance of metadata parameters

        Parameters
        -----------
        layout : str, optional
            Sets the layout name for this request. This takes precedence over
            the value stored in the Server instance's layout attribute
        metadata : str, optional
            Options to get all or specifics metadatas.
            Choices are 'fields', 'portals', 'value_lists' or 'all'.
            Default is 'fields'
        """
        target_layout = layout if layout else self.layout
        path = self._get_api_path('meta.layouts') + f'/{target_layout}'

        response = self._call_filemaker('GET', path)

        if metadata == 'all':
            return response
        elif metadata == 'portals':
            return response.get('portalMetaData', None)
        elif metadata == 'value_lists':
            return response.get('valueLists', None)
        else:
            return response.get('fieldMetaData', None)

[davidhamann]

I would suggest renaming get_layout_valueList to get_value_list_values such that it's clear that we don't fetch meta data for the value list (as we might want to do later on), but only the values itself.

[LeJeko]

Ok

[davidhamann]

You write "returns a list of..." but at the end return a tuple. I think returning a list of tuples would actually be a good approach (i.e. [("a", "A"), ("b", "B"), ...]) such that users could still modify the returned list without converting it back list again.

Or was there a specific reason for using a tuple of tuples vs. a list of tuples?

[LeJeko]

I agree with having a list of tuples as the default output. Therefore, I suggest adding a parameter with the options 'list' (default) and 'tuple' to choose the output format.

In my case, I use it with Django for the CHOICES of a form SELECT, as in a FileMaker Pop-up. For this usage, I specifically need a tuple of tuples.

[davidhamann]

Outdated comment for fms?

[LeJeko]

Correct, because I now use 'self' instead.

[davidhamann]

Please use snake case for the variable name, i.e. value_lists.

[LeJeko]

Ok

[davidhamann]

I think we could speed up the loop where you identify the right value list with an early return or break. Something like this (you don't have to use a list comprehension, only the break really makes the difference):

for vlist in value_lists:
    if vlist['name'] == name:
        values += [(v['value'], v['displayValue']) for v in vlist['values']]
        break

[LeJeko]

yes, well done!

[davidhamann]

I would prefer not to reference a potential use case (e.g. building the list for an HTML form, or such) in the method since this would be an implementation detail of the user. They might use it to populate a form, or do something completely different with the returned data.

[LeJeko]

ok, got it

[LeJeko]

Thanks for your comments. See replies.

[davidhamann]

Thanks for your work! Looks all good to me. Just two small things:

1.) Can you put the date keyword logic into its own method, such that we have this logic only at one place? Something like this:

def _date_format_for_keyword(self, keyword: str) -> Optional[str]:
    return next((format[1] for format in API_DATE_FORMATS if format[0] == date_format), None)

2.) I would prefer not to have an output parameter for the get_value_list_values method as it's a specific use-case, would require a union return type (can then be a list or a tuple) and is just one more function call in users' code. Would it be possible for you, to just do tuple(fms.get_value_list_values(...)) in your code vs. in the library?

Thanks again for your work. Happy to merge with these last changes.

[LeJeko]

I agree with your comments. Code committed.

[davidhamann]

Merged, thanks!