bt5: fix naming convention for FieldValidator_requireIfDialogConfirmed
FieldValidator is not an acceptable prefix, Base_validateDialogConfirmation seems better
-
Developer
Indeed better naming, but this change can break compatibility with projects already using it, so it could worth to check
-
Owner
That's a good point, thank you.
I'm thinking we could address this by extending "coding style" test by a test checking that all fields that all validators defined on fields really exist. It's not really "coding style" here, but this is where we do all this kind of check of configuration. I can do this, let me know what you think about it.
-
Developer
It's not really "coding style" here, but this is where we do all this kind of check of configuration.
indeed it is not. I am not too familiar with this what this test includes, but if you believe it fits there it is ok for me.
Other idea would be to keep also
FieldValidator_requireIfDialogConfirmed
logging a deprecation warning. Not that I'd be too happy with duplication, just proposing. -
Owner
Yes this was also one idea I had, to keep a
FieldValidator_requireIfDialogConfirmed
in the bt for obsolete proxy field compatibility, but it's probably something where we can break compatibility, as long as there's a test failing. This kind of UI things are often not tested, so having a test checking the field configuration looks the most realistic way to have a test failing when this does not work. -
Owner
I started next step, which will be check that ERP5 Form are correctly named ( !1021 (merged) ), and I thought about another approach maybe instead of fixing old names we should just add all old names to a list of ignored .
-
Developer
This kind of UI things are often not tested, so having a test checking the field configuration looks the most realistic way to have a test failing when this does not work.
+1
I started next step, which will be check that ERP5 Form are correctly named ( !1021 (merged) ), and I thought about another approach maybe instead of fixing old names we should just add all old names to a list of ignored .
OK, thanks
-
mentioned in merge request !1031 (merged)
-
Owner
This kind of UI things are often not tested, so having a test checking the field configuration looks the most realistic way to have a test failing when this does not work.
+1
Thanks for the feedback, I made !1031 (merged)