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

allow different "ignore_discard" value at save() time #2

Merged
merged 2 commits into from Nov 28, 2019

Conversation

@Lx
Copy link
Contributor

Lx commented May 3, 2013

This change set provides the save method with an additional optional ignore_discard argument to override the value of the ignore_discard object attribute for this particular save operation.

Addresses [rt.cpan.org #85011].

lib/HTTP/Cookies.pm Outdated Show resolved Hide resolved
@oalders

This comment has been minimized.

Copy link
Member

oalders commented Mar 21, 2019

Thanks for this! I just pushed a change. If you rebase from master the tests should start running again.

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 22, 2019

I've rebased from master and fixed the careless bareword issue (sorry—I was fixing the merge conflict directly on GitHub).

I've basically recreated the branch from scratch, cherry-picking the necessary commits. I hope I haven't made things worse.

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Mar 22, 2019

Sorry, now that tests are passing and I'm having a closer look at this, I'm wondering if it wouldn't be easier to have an ignore_discard() method to get and set the value. I see that was one option you proposed in the referenced ticket. I think having a method to do this would make the API clearer. I find passing the second param a bit less clear. Having an ignore_discard() method would make it immediately clear what is happening. @genio what do you think?

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 22, 2019

I consider that the method as proposed in this PR is better for these reasons:

  1. It doesn't alter object state (and therefore won't cause unintended side effects on possible later calls to save).

  2. Python already does it this way—although admittedly Python's named arguments help to make what's happening very clear.

  3. One can alter the behaviour of a save in a one-off manner without having to wrap the save in a construct like:

    my $old_ignore_discard = $cookie_jar->ignore_discard();
    $cookie_jar->ignore_discard(0);
    $cookie_jar->save($file);
    $cookie_jar->ignore_discard($old_ignore_discard);
    
@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 22, 2019

I'd also argue that changing the behaviour of save to depend on a modifiable ignore_discard object attribute breaks backwards compatibility, since legacy code won't be treating cookie jar instances as per Point 3 above.

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 22, 2019

One final similar alternative which would improve clarity at the call site would be to accept a hash/hashref instead of a positional argument, i.e. one of:

$cookie_jar->save($file, ignore_discard => 0);
$cookie_jar->save($file, { ignore_discard => 0 });

although if someone writing code is so concerned that the call as currently proposed in the PR can't be understood without reading the docs, they could always use constants:

use constant DONT_IGNORE_DISCARD => 0;
$cookie_jar->save($file, DONT_IGNORE_DISCARD);
@oalders

This comment has been minimized.

Copy link
Member

oalders commented Mar 25, 2019

I see what you're saying about ignore_discard() => save() => ignore_discard(). Personally, I think $cookie_jar->save($file, { ignore_discard => 0 }); is the most readable option but let's see if we can't get another opinion in here.

@oalders oalders requested a review from genio Mar 25, 2019
@genio

This comment has been minimized.

Copy link
Member

genio commented Mar 29, 2019

I think it would be best to go ahead and have a method for all attributes for getting/setting, and if we're going to introduce overrides into the save method, I prefer @oalders method of using a hash/hashref rather than a singular parameter

@genio

This comment has been minimized.

Copy link
Member

genio commented Mar 29, 2019

I guess what I'm trying to describe would be akin to:

my $jar = LWP::Cookies->new(ignore_discard => 1);
say $jar->ignore_discard(); # 1
$jar->ignore_discard(0);
say $jar->ignore_discard(); # 0

$jar->save($file, ignore_discard => 1);
say $jar->ignore_discard(); # 0 still, the override above was just for the save process
@genio

This comment has been minimized.

Copy link
Member

genio commented Mar 29, 2019

Though I'm still not sold on the overriding of attributes during the save method. I tend to think that makes the save method overly complex.

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 29, 2019

I disagree that having the logic in save is overly complex. Not having the logic in save (2–3 lines) just moves the complexity to the call site—and actually amplifies it (4 lines) if the ignore_discard attribute only needs to be a certain value during the save and then reverted to the previous value (see example further above).

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 29, 2019

Actually, reverting becomes even more complex without the logic in save, because one needs to cater for the possibility that save fails and still ensure that the value reverts. So my example above is incomplete and should actually also adopt eval and $@.

All of this call site complexity can be avoided by allowing ignore_discard as a save argument in one form or another.

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Mar 29, 2019

I think doing this via a getter/setter makes the intent of the code more obvious. It seems to me to be the least surprising way of doing this. It will be more verbose, but that should also make very clear what is going on.

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 30, 2019

I must afford you the respect of some blatant frankness here: I am finding this ongoing discussion completely disheartening and dismissive of actual, real-life needs.

I wrote this PR as an actual user of this library, I have posed my justifications as an actual user of this library, and I have demonstrated that Python’s equivalent library implements an ignore_discard argument on save (presumably due to some specific, accepted real-life use case—like the one described further above).

Making your users jump through massive hoops for one-off saves of discardable cookies for the argument of “clarity” is, with all due respect, dubious.

I wouldn’t be sitting at my desk admiring how “clear” this API makes my code look if what should be a one-line call blows out to a get, two sets, a temporary variable assignment, an eval to revert in the case of I/O error (or otherwise the risk of leaving the object in an inconsistent state) and finally the actual work.

This can all be avoided with minimal API-side complexity with a save argument (positional as currently coded, or named which I’m happy to alter to).

@genio

This comment has been minimized.

Copy link
Member

genio commented Mar 30, 2019

@Lx We are obviously missing each other's point somewhere along the way here. I'm not quite sure I understand the workflow you're trying to explain, and you're not understanding why I (and seemingly others) are missing your point on why such an API change would be beneficial.

Might I suggest we talk on the IRC Channel So that the discussion here doesn't take a sour turn and we can all get on the same page?

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 30, 2019

I strongly prefer keeping all discussion on this matter as centralised as possible (it's already spread across this PR and Issue #23) so I'll try to re-cover all bases here.

BTW, please don't interpret my directness as sourness—I'm simply trying to be as honest & direct as possible about my position to mitigate the misunderstandings we seem to have between us, and I appreciate honest, direct responses in return.

Please also keep in mind my perspective, which is someone who has had to maintain workarounds for almost six years due to this issue, to then be told (after going to the trouble of a merge) that their contribution is totally unacceptable for what appear to be unfounded reasons.

The current situation

  • The ignore_discard object attribute is set at cookie jar construction time.
  • It is exclusively used during save operations.
  • It is not readable or writable after construction.
  • One cannot tell from looking at consumer calls to save whether discardable cookies will be saved or ignored.

The problems

  • Because the ignore_discard attribute is only used during save operations, the historical decision to make this an object attribute in the first place is questionable—it could have always just been an optional argument to save with a sensible default (Python does this). Nonetheless, for backwards compatibility, it must now forever remain as a construction-time setting—but this historical decision should not preclude improved future usability.
  • As a consumer of cookie jar objects I occasionally need to save all cookies to file, including cookies that would normally be discarded.
  • I don't always have control over the construction of the cookie jar objects I'm working with.
  • I can't replace the cookie jar objects with ones I've constructed, and which have the ignore_discard setting I need.
  • I am resorting to breaking object encapsulation, changing object state, and protecting object state reversion through eval and re-throwing as needed (see Code Example 1 below, but with direct hash manipulations on the cookie jar object instead of method calls).

Potential solutions and thoughts on each

  1. introduce an ignore_discard method to get/set the object attribute after construction

    • the currently-preferred solution by the maintainers because:
      • it avoids “overly complex” code in save—which is very subjective and dubious (see further below)
      • it avoids “overriding of attributes during the save method”—however the attribute it overrides exclusively concerns the save method anyway
      • more verbose code at the call site increases clarity—which is simply not true compared to using a named argument on the save method call
    • if implemented alone, requires consumers to write complex code involving gets, multiple sets, and eval/re-throw to prevent the object state change from “leaking out” if an I/O error occurs (as per Code Example 1 below)
    • the code and tests are not written
    • further entrenches the use of an object attribute for something that shouldn't be one (again as supported by Python's implementation)—although I fully own that this is opinion and not fact
  2. introduce an optional ignore_discard argument on save which defaults to the object-based ignore_discard object attribute (and optionally deprecate the object attribute altogether)

    • the currently-preferred solution by me
    • allows for significantly simpler code at the consumer side when needing to save discardable cookies one time from a cookie jar object not constructed by the consumer (see Code Example 2 below)
    • Python's preferred and currently-implemented solution—in fact Python doesn't even offer an object attribute for this functionality, hence the deprecation suggestion
    • the added complexity to the save method is one line (two with a named argument)
    • promotes away from using the ignore_discard object attribute at all
    • the code and tests are already written (for positional argument—happy to change to named argument)

The misunderstandings

  • The maintainers don't understand why I don't see Solution 1 as acceptable for a consumer who has to sometimes concern themselves with discardable cookies
  • I don't understand why the maintainers don't see Solution 2 as vastly superior for consumers

Code example 1

The full code to temporarily change the ignore_discard value for one save under Solution 1 (and propagate I/O errors without leaving the object in a modified state) is:

local $@; # don't reset $@ for caller
my $old_ignore_discard = $cookie_jar->ignore_discard();
my $eval_ok = eval {
    $cookie_jar->ignore_discard(0);
    $cookie_jar->save($filename);
    1; # to indicate no error
};
my $error;
if (!$eval_ok) { # don't trust $@ as it might have been reset as the eval unrolled
    # copy $@ in case resetting ignore_discard value resets $@
    $error = $@ || 'unknown error';
}
$cookie_jar->ignore_discard($old_ignore_discard);
if ($error) {
    die $error;
}

Code example 2

The full code to temporarily change the ignore_discard value for one save under Solution 2 (and propagate I/O errors without leaving the object in a modified state) is:

$cookie_jar->save($filename, ignore_discard => 0);
@genio

This comment has been minimized.

Copy link
Member

genio commented Mar 30, 2019

Thank you for the thorough breakdown. I think we're on the same page and I see your use case clearly.

I do, however, have some comments on your code examples and some explanation about what we mean by complexity.

No matter the route chosen, a try/catch of sorts (using eval) will be required as the save method will throw an exception (die) on error, so your example number 2 is a bit oversimplified, making example number 1 look less appealing.

Example 1:

my $cur_ignore = $jar->ignore_discard();
my $error;
{ # catch block
    local $@;
    $error = $@ || 'Error' unless eval {
        $jar->ignore_discard(0)->save($file);
        1
    }; # try
}
die "Error when saving: $error" if $error;
$jar->ignore_discard($cur_ignore);

Example 2:

my $error;
{ # catch block
    local $@;
    $error = $@ || 'Error' unless eval {
        $jar->save($file, ignore_discard => 0);
        1
    }; # try
}
die "Error when saving: $error" if $error;

The Complexity Mentioned:

In Perl, we typically have multiple arguments for a method passed as a hash/hash reference. So, if we did add such a concept here to the save method, since the $file is already the first parameter, we'd have to do it somewhat like this (untested and typed in GitHub's textarea, so there's likely an error here or there):

sub save {
    my $self = shift;
    my $file = shift;
    my %args;
    if ($file && CORE::ref($file) eq 'HASH') {
        %args = %{$file};
        $file = $self->{file};
    }
    return unless $file;
    if (!%args && @_) {
        if (@_ == 1 && $_[0] && CORE::ref($_[0]) eq 'HASH') {
            %args = %{$_[0]};
        }
        elsif (@_ % 2 == 0) {
            %args = @_;
        }
    }

    my $ignore = $self->{ignore_discard};
    $ignore = $args{ignore_discard} if exists($args{ignore_discard});

    open(my $fh, '>', $file) or die "Can't open $file: $!";
    print {$fh} "#LWP-Cookies-1.0\n";
    print {$fh} $self->as_string(!$ignore);
    1;
}

We could then argue the merits of using a hash/hashref as the next parameters, but that's what we're referring to as complexity.

On the other hand a mutator method is quite simple by comparison:

sub ignore_discard {
    my $self = shift;
    return $self->{ignore_discard} unless @_;
    $self->{ignore_discard} = $_[0] ? 1 : 0;
    return $self;
}

Again, now that we're on the same page about what it is that you're looking to accomplish, we'll discuss it some on IRC and see what everyone thinks. It's hard to introduce API changes that would break things (removing it as an attribute all together) but as you say, there's certainly merit in exposing the attribute in some way.

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Mar 30, 2019

I fear that the reason for exception propagation appearing in only one of my code examples is not fully understood.

Please assume that both code examples appear in functions at the non-main level, and that they expect something else to do any error handling for the main process.

The exception handling/propagation in my first example exists solely to ensure that the temporary state change of the cookie jar is reversed when a failure occurs, while still allowing that failure to propagate.

The second code example lacks any exception propagation because there is no temporary object state change to reverse in the case of a failure, and thus no need to catch an error—just let it propagate naturally.

For this reason, it’s important to acknowledge that the difference in complexity between my two examples (specifically with regard to error catching & propagation) is accurate. Your Example 2 doesn’t serve any purpose by catching and re-throwing the error, making most of its code redundant.

Regarding the maintainers’ complexity concerns, while I definitely appreciate your explanation, it runs on the assumption that you would also want to cater for an existing positional argument, which wasn’t conveyed in the example given to me in an earlier comment:

$jar->save($file, ignore_discard => 1);

To support the example line directly above, the complexity would be vastly simpler than what you’ve written—although I definitely concede that if you did want to enhance the save API to accept a hashref as the first argument instead of a filename string, and retain backwards compatibility, this would add significant complexity.

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Mar 30, 2019

@autarch, could I trouble you for a quick opinion on this API change? If you're busy, then don't worry about it, but I wouldn't mind hearing someone else's thoughts on how best to go about this.

@autarch

This comment has been minimized.

Copy link

autarch commented Mar 30, 2019

I agree with @Lx. Making this a get/set attribute is gross and leads to complex code solely to preserve the original state of this attribute. If you otherwise are fine with ->save throwing an exception (which I imagine you generally are) then it's even grosser.

Also, mutable state is the root of all evil.

I do, however, have one other slight variation on the proposal. Instead of allowing named params in some form after the filename, just allow all named params or a single file param:

$jar->save($file);
# or
$jar->save( file => $file, ignore_discard => 1 );

This can be handled in the method with fairly minimal code:

sub save {
    my $self = shift;
    my %args = @_ == 1 ? ( file => shift ) : @_;
    $args{ignore_discard} = $self->ignore_discard unless exists $args{ignore_discard};
    ...
}

That 3rd line to set $args{ignore_discard} isn't the most elegant, but it seems readable enough to me. Of course, you really should do some error-checking, so you'd want something more like this:

my %args = @_ == 1 && !is_hashref($_[0]) ? ( file => shift ) : validate_somehow(@_);

As for what to validate with, well, I can point you to several modules I've written ;) Or you could hand-code it easily enough (though tediously). And you should of course handle either a hashref or list of named params.

Note that validation is a concern no matter how you allow additional args, because once you have named args you really do want to provide users the consideration of catching their typos.

@autarch

This comment has been minimized.

Copy link

autarch commented Mar 30, 2019

Here's another way to think about it. Once we know that there is a solid user case for passing ignore_discard each time you call ->save it becomes apparent that making this an object attribute was probably a mistake. That means you should support this new argument in the nicest way possible while preserving the backwards compatibility of setting it in the constructor. Making it mutable is just doubling down on the original API design mistake.

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Mar 30, 2019

@Lx are you willing to implement what @autarch has suggested? That works for me. (Thanks @autarch!)

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Apr 3, 2019

I'm happy with the outcome of this discussion. I'll implement as @autarch has suggested. Thank you all for your time & thoughts.

@genio

This comment has been minimized.

Copy link
Member

genio commented Apr 3, 2019

@Lx This would be something that would be an acceptable approach to what @autarch suggests.

sub save {
    my $self = shift;
    my $args;
    if (@_ == 1 && ref $_[0]) {
        my %copy = eval { %{ $_[0] } }; # try shallow copy
        die "Argument to method could not be dereferenced as a hash" if $@;
        $args = \%copy;
    }
    elsif (@_==1 && !ref($_[0])) {
        $args = {file => $_[0]};
    }
    elsif (@_ % 2 == 0) {
        $args = {@_};
    }
    else {
        die "Method got an odd number of elements";
    }
    my $file = exists($args->{file}) ? $args->{file} : $self->{file};
    return unless $file;
    my $ignore = exists($args->{ignore_discard}) ? $args->{ignore_discard} : $self->{ignore_discard};
    open(my $fh, '>', $file) or die "Can't open $file: $!";
    print {$fh} "#LWP-Cookies-1.0\n";
    print {$fh} $self->as_string(!$ignore);
    1;
}
@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Apr 7, 2019

I've implemented the API suggested by @autarch above, which was:

$jar->save($file);
# or
$jar->save( file => $file, ignore_discard => 1 );
  • I've also updated HTTP::Cookies::Netscape->save, which I missed entirely the first time.
  • In doing so, I identified a discrepancy in saved output of HTTP::Cookies vs. HTTP::Cookies::Netscape—the former saves expired cookies while the latter does not. I'm unsure whether the maintainers would consider this a bug. I worked around it by ensuring that the Netscape test sets a future expiry date.
  • I've deliberately not implemented acceptance of a single hashref, because the unwanted complexity outweighs the benefit, and the example usage in the docs clearly shows that a hash is expected.
  • I was hesitant to put in any validation since no such validation takes place elsewhere (e.g. the constructor), but I've put in a check for unexpected hash keys which should cover most accidental misuses.
@oalders
oalders approved these changes Apr 8, 2019
@genio

This comment has been minimized.

Copy link
Member

genio commented Apr 8, 2019

Looks good. I will try to test fully and merge tonight

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Jun 16, 2019

Hi. Just checking in on this. Did you need anything more from me for this to get merged?

@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Nov 27, 2019

I see that three distinct versions of this library have been released since the changes in this PR were approved, which of course is appreciated.

I get that everyone has lives outside of coding and that this library is entirely a volunteering collaboration, and it is not my intention to embarrass anyone unduly, but in the spirit of blatant frankness, I must say I'm incredibly frustrated by the sheer amount of resistance I've faced in resolving this issue. This whole experience for me has been remarkably terrible and unrewarding.

Can I ask you to please consider my perspective for a moment to understand my frustration?—

  • I took the time to write a working, mergeable patch in 2013 (as opposed to just complaining and expecting others to do the job).
  • The patch was ignored completely for almost six years amid multiple releases.
  • I subsequently had to justify my change to the nth degree multiple times in the comments above.
  • I then had to make the code ready to merge again (and added a bonus fix to a neighbouring issue).
  • It got approved but not merged.
  • More releases are happening, making my code at risk of becoming unmergeable a second time.
  • On top of everything above, my last comment was totally ignored.

I have invested hours of my life into this issue and I am only asking at this point for a few minutes in return to complete this merge. Please, can we make this happen soon so that all of the time that has been invested into this issue (including yours!) isn't completely wasted?

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Nov 28, 2019

@Lx I understand your frustration. You can see that I did some releases of this module in the meantime so you rightly have been left wondering why this was never merged. I think in the chaos it just got lost. I had some serious life events in late June and I had to drop a lot of things. I'm getting back to a normal routine now, but I had to let a lot of stuff go and open source was one of those things.

I have a lot of different projects to track and, honestly, just asking "why has nobody looked at this?" is a really good way to get things moving again. I have recently created a project at https://github.com/orgs/libwww-perl/projects/1 for us to be better at tracking in progress work so that we're not going to lose things.

As an org we are understaffed and slowly trying to chip away at PRs that have been neglected for years. See libwww-perl/libwww-perl#78, which is not quite as old as your (it's from 2015), but that just got closed in the last week or so. I have seen my own PRs in other projects go neglected for up to 5 years -- sometimes just simple doc fixes. Sometimes they don't get merged at all or sometimes they silently get merged without so much as a thank you. Open source is a weird thing. I know there was some pushback on your code. Sometimes we do need to be extra sure of things because of how many downstream modules can be affected and also because occasionally someone will "fix" something and disappear and we are then left to clean up the mess. So, we try to be conservative and make sure there's a compelling reason for a change.

Anyway, I will follow up with @genio and see where he left things. We will get this sorted. One of the reasons I got involved with this project was because so many code contributions had gone neglected for so long and that's an unfortunate state of affairs. If we had more help, we could get more done, but we are doing the best we can with the resources that we have.

Thanks very much for your contribution. It's appreciated, even if it may not seem like that right now. :)

@genio
genio approved these changes Nov 28, 2019
@genio genio merged commit 33c7ddb into libwww-perl:master Nov 28, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Lx

This comment has been minimized.

Copy link
Contributor Author

Lx commented Nov 28, 2019

Thank you both. ❤️

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Nov 28, 2019

Thanks @genio (and Happy Thanksgiving)! I've added an item to the group project to release a new HTTP::Cookies. I'll likely do that on Monday rather than shipping new code on a long holiday weekend. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.