Do not overwrite existing names #1568
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1568 +/- ##
=========================================
Coverage 97.47% 97.48%
Complexity 987 987
=========================================
Files 205 205
Lines 2221 2227 +6
=========================================
+ Hits 2165 2171 +6
Misses 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
henriquemoody
force-pushed
the
core/name
branch
2 times, most recently
from
836fa77 to
d679872
Compare
This commit addresses the skipped tests by modifying certain core concepts in the library. The way names work has always been somewhat confusing, even to me. I’ve established that existing names will never be overwritten, and if a path is already defined, we will change the name to also include the path. I’m not very happy with how I’m solving this problem, because the `FirstResultStringFormatter` is changing some behaviour in the results that seems to be something that should always happen before. But, for the moment, I will keep it as is.
henriquemoody
force-pushed
the
core/name
branch
from
d679872 to
8332d28
Compare
| // Message: `.foo` (<- Custom name) must be present | ||
| ``` | ||
|
|
||
| If you want to display only a custom name while checking if a key exists, use [Key](Key.md) with [AlwaysValid](AlwaysValid.md): |
There was a problem hiding this comment.
I'm wondering if we shouldn't rename Key and Property to KeyValue and PropertyValue, since now they require the value to be present.
Also, while the text is informative, it might be difficult for users to understand when to pick Key, KeyExists and KeyValue just from reading the description.
I was thinking maybe we should do a separate markdown file with some sort of direct comparison (maybe even a table?) of these very similar rules (Key, KeyExists, KeyOptional, Property, PropertyExists, PropertyOptional).
Hopefully, by comparing them directly in a single document, we can better show their differences to users. We can then do a small link here to that doc file (same with Property).
WDYT?
There was a problem hiding this comment.
I'm not a big fun of renaming the rule because I find it would be too lengthy.
I do agree that users will need more information on those structural rules and I like the extra documentation and the table you suggested!
alganet
left a comment
alganet
left a comment
There was a problem hiding this comment.
The comment I left on the docs is non-blocking and an improvement that we can later work on. Overall, seems good!
2 participants
This commit addresses the skipped tests by modifying certain core concepts in the library. The way names work has always been somewhat confusing, even to me. I’ve established that existing names will never be overwritten, and if a path is already defined, we will change the name to also include the path.
I’m not very happy with how I’m solving this problem, because the
FirstResultStringFormatteris changing some behaviour in the results that seems to be something that should always happen before. But, for the moment, I will keep it as is.