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

Render/suggest empty custom vars correctly #779

Merged
merged 6 commits into from Jul 5, 2023
Merged

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Jun 19, 2023

fixes #778

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Jun 19, 2023
@yhabteab yhabteab requested a review from nilmerg June 19, 2023 13:09
@yhabteab yhabteab force-pushed the fix-empty-custom-vars branch 2 times, most recently from 4be4d0c to f757827 Compare June 19, 2023 13:32
@yhabteab yhabteab force-pushed the fix-empty-custom-vars branch 2 times, most recently from 7261ff0 to afd50fd Compare June 22, 2023 11:03
@yhabteab yhabteab requested a review from nilmerg June 22, 2023 11:17
@nilmerg nilmerg added bug Something isn't working area/representation Affects the representation of information labels Jun 22, 2023
@nilmerg nilmerg added this to the 1.1.0 milestone Jun 22, 2023
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

vars = {
    examples["Joooo"] = []
    examples["Joooo"][0] = []
}

image

@yhabteab yhabteab force-pushed the fix-empty-custom-vars branch 2 times, most recently from 58d8183 to 7575479 Compare June 23, 2023 07:40
@yhabteab yhabteab requested a review from nilmerg June 23, 2023 07:41
@yhabteab yhabteab force-pushed the fix-empty-custom-vars branch 3 times, most recently from 39ecc74 to 2e2b670 Compare June 23, 2023 10:01
@yhabteab yhabteab requested review from nilmerg and removed request for nilmerg June 23, 2023 10:02
@yhabteab
Copy link
Member Author

Bildschirmfoto 2023-06-23 um 12 16 22

library/Icingadb/Model/CustomvarFlat.php Outdated Show resolved Hide resolved
library/Icingadb/Model/CustomvarFlat.php Outdated Show resolved Hide resolved
@julianbrost
Copy link

One thing I noticed in the screenshot: empty arrays seem to miss the "(Array)" hint.

@nilmerg
Copy link
Member

nilmerg commented Jun 26, 2023

That's unavoidable. PHP cannot differentiate between an empty dict and an empty list.

@julianbrost
Copy link

Unavoidable as in you're reconstructing the whole vars data structure with PHP data types and Icinga 2 arrays and dicts are both represented by PHP arrays? And if there's an empty PHP array, you don't know whether it was an array or dict in Icinga 2?

@nilmerg
Copy link
Member

nilmerg commented Jun 26, 2023

@yhabteab Please check once more. Should be working fine now.

@nilmerg
Copy link
Member

nilmerg commented Jul 5, 2023

Didn't look at your/my code changes yet, but testing revealed this:
image

@nilmerg nilmerg merged commit 7e044d2 into master Jul 5, 2023
12 checks passed
@nilmerg nilmerg deleted the fix-empty-custom-vars branch July 5, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/representation Affects the representation of information bug Something isn't working cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Render empty customvars correctly
3 participants