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

Add template typedef review usage test #1502

Merged
merged 22 commits into from
Sep 27, 2024

Conversation

dbauszus-glx
Copy link
Member

@dbauszus-glx dbauszus-glx commented Sep 25, 2024

_type is a property of the template typedef addded in this PR.

I merged updateTemplateUsage and checkTemplate to become the documented update_templateUsage method.

The undocumented updateTemplateUsage method was only called by the undocumented checkTemplate method which in turn updated the templateUsage param.

There are still a few things I want to check.

Having a custom_templates object and templateUsage object which are essentially the same but the latter has a counter feels odd.

The templates object can just be cloned with the count being added once.

It doesn't feel right to iterate through layers in locales to update_templateUsage on each iteration within an iteration.

The update usage should be done once after all locales and layers have been parsed.

I am worried that templates used in an entry, or dataview in a layer do not count to the usage if we only check the layer.templates and layer.template.


@dbauszus-glx dbauszus-glx added the Documentation Adding or updating documentation. label Sep 25, 2024
@RobAndrewHurst
Copy link
Contributor

//We get the initial custom templates from the workspace.templates as we want to see usage of templates before we merge templates from layers.
    custom_templates = {
      ...Object.fromEntries(
        Object.entries(workspace.templates).filter(([key, value]) => value._type === 'workspace_template')
      )
    };

We get the initial custom templates from the workspace.templates as we want to see usage of templates before we merge templates from layers.

I added in this comment where I get the workspace_templates from the workspace.templates object before we merge the layer templates as we want to see what's explicit templates are in the workspace.templates that either need to remain or be removed.

If a template is being merged from a layer then chances are it's something that is needed for the workspace. But if its just old legacy config that is lying around this could help us 'couper le gras'.

If we have other ideas in how we could handle this, then lets discuss it :)

@RobAndrewHurst
Copy link
Contributor

#1494

I have updated the templates test spec with additional things we discussed.

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

It is valid to have no workspace.templates object, for example if all the templates are defined in the locale.layers and/or on layers themselves.

This currently crashes in the cache.js module as there is no check on the presence of workspace.templates.

@dbauszus-glx dbauszus-glx marked this pull request as ready for review September 26, 2024 13:16
@simon-leech
Copy link
Contributor

   "template": {
          "key": "metrics_dwelling_privaterented_perc_thematic_2021",
          "template": "1-999"
        },

With a configuration as above, the used_templates array will contain

"used_templates": [ 
"1-999": 1
]

for example. It should use the key of the template instead

@dbauszus-glx dbauszus-glx linked an issue Sep 26, 2024 that may be closed by this pull request
Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

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

Nice one! I am happy with this!

@dbauszus-glx
Copy link
Member Author

The template object: key should not be removed from unused templates and added to usage statistics as these are added to the templates object and not immediately used. This prevents double counting.

Instead the key should be added to the overwritten_templates set if the template existed as a _type=workspace template in the first place.

@simon-leech
Copy link
Contributor

Just pushed a commit to sort the templates so related naming of templates are seen together - this is working as expected for me now - thank you!

Copy link

@RobAndrewHurst RobAndrewHurst merged commit 7a7a20d into GEOLYTIX:main Sep 27, 2024
5 checks passed
@dbauszus-glx dbauszus-glx deleted the template-type-usage-test branch November 27, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Adding or updating documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unused Template Check
3 participants