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

Support add/add_item method chaining #141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

happy-barney
Copy link

Particularly useful when there is one item only or
static configuration map (like in http-config.t)

Particularly useful when there is one item only or
static configuration map (like in http-config.t)
@happy-barney
Copy link
Author

Run Tests with coverage failed with in cover -report codecov call

400 Bad Request
{"meta": {"status": 400}, "error": {"reason": "Please provide the repository token to upload reports via `-t :repository-token`", "context": null}}

@vanHoesel
Copy link
Member

I would be very hesitant doing this, depending on any calling code, it used to return something undef-ish. With this code change, it would return $self. Calling code may get confused here.

Copy link
Member

@vanHoesel vanHoesel left a comment

Choose a reason for hiding this comment

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

This may break calling code, that would normally expect something undef-ish and now get a $self back

@vanHoesel
Copy link
Member

I understand your desire for making it chainable.

May I suggest to use with_item that would actually return a cloned version with the additional item. Such that the original would remain un-mutated. Inmutabillity is great to have. Would love to kill the add_item :-)

@happy-barney
Copy link
Author

To be honest, I don't understand this immutability obsession. Like every tool, every approach, it has limitations ... your proposal's limitation is immediately in this pull request, see modified test.

Honestly I don't care about this, I'm not using this module, I just recalled it while doing karenetheridge/Test-LWP-UserAgent#4 (that one I use) IMHO itt will just make user's live easier.

@vanHoesel
Copy link
Member

My main concern is not about immutability (although that is very good to have)

My great concern is that this change will affect the behaviour of the add call, where it did not return anything in the past, it now would return the invocant. Calling code may very well not anticipate on such change This proposed change may very well have dramatic impact on applications and systems out there in the wild. This change may possibly break production code.

I, myself, have witnessed breakage because of code in the wild, because of proposed and approved changes in the HTTP realm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants