Closed igorissen closed 7 months ago
Thanks for the PR.
While I agree that the clear
option for get()
has understanding issues when reading the code, your fix is not the correct solution (supposing it is a big enough issue to warrant a fix).
pop()
is normally used on arrays (with no argument), and using it for a key-value store adds confusion. Additionally, you would expect to have the counterpart push()
available.
If we want to fix this, the simple solution is probably to change it to an option
argument with a clear
parameter. Used like:
request.yar.get('key', { clear: true });
The even simpler solution, is to remove the option all together, requiring users to explicitly call clear()
. The option is a pure developer convenience:
request.yar.get('key');
request.yar.clear('key');
I'm probably in favour of this approach.
While we are at it, we might want to rename clear
(both as an option, and the method) to delete
, to better match the Map
semantics, where clear()
removes all elements.
The even simpler solution, is to remove the option together, requiring users to call clear explicitly ().
While we are at it, we might want to rename clear (both as an option and the method) to delete, to better match the Map semantics, where clear() removes all elements.
I'm also in favour of the removal of the options parameter. To have a simple function with one goal will be simple to understand and even simpler to test. The developers could create themselves a function which calls the get
and delete
.
@kanongil do I need to create a new feature request issue or update my current feature request issue? or should I do the modification and create a PR?
:unicorn: Context
When we first used this plugin, we wondered if there was a way to retrieve and delete a value. We found in the documentation that the
get
method accepts an additional parameter, which is the delete option after retrieval.For new developers on the project or people who will be reviewing the code, a recurring question we've noticed is what the second parameter of the
get
method does?It could be great to have a method with more clarity like Array.prototype.pop() to retrieve and delete a value.
:robot: Proposal
Add a new method with the name
pop
which retrieves and removes the value associated with the specified key from the request yar store.:rainbow: Notes
N/A
:100: Tests
npm test
and/ornpm run test-cov-html