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

fix unescape #18

Merged
merged 2 commits into from
Aug 28, 2019
Merged

fix unescape #18

merged 2 commits into from
Aug 28, 2019

Conversation

Madskill
Copy link
Contributor

Hello!

I think that function "unescape" can't correctly work with string \\".
This PR fix it.

But test can't work on master branch... Can you see my PR?

@renormalist
Copy link
Owner

I see your PR, thanks. But I'm just heading for vacation, I will try next week.

What do you mean with "test can't work on master branch"?

Kind regards,
Steffen

@Madskill
Copy link
Contributor Author

Ok, thanks.

I do this:

  1. git clone ...
  2. git checkout master
  3. run tests prove -l ./lib ./t

Result: some tests failed

It's ok?

@renormalist
Copy link
Owner

Well, which tests, that's the important part. :-) If they are related to your \" escaping then maybe. I will have a look in a week.

@renormalist
Copy link
Owner

Ah, I just found your gist about this issue in my spam folder. Now at least I see what problem you mean...

@Madskill
Copy link
Contributor Author

Madskill commented Nov 5, 2018

@renormalist renormalist
Hi, when this pull request will be merged?

@renormalist
Copy link
Owner

I'm again staring at the change, but I'm still unsure due to the other failing tests which essentially could break other libs using dpath. Can you please have a look at #7 to see if that is the same problem you have? Thanks.

@renormalist
Copy link
Owner

@Madskill
Copy link
Contributor Author

Madskill commented Nov 24, 2018

Hi @renormalist , this PR is OK. It is fix bug...
I find that this PR #16 failed tests on master...

See

  1. check tests without branch "use_list_util"
> git checkout v0.57

> prove -I./lib ./t
t/00-load.t .................. ok   
t/basics_without_overload.t .. ok     
t/cyclic_structures.t ........ ok   
t/data_dpath.t ............... ok     
t/iterator.t ................. ok    
t/matchr.t ................... ok    
t/optimization.t ............. ok   
t/parallel.t ................. ok    
t/path.t ..................... ok     
t/references.t ............... ok    
t/regressions.t .............. ok   
t/zeros.t .................... ok    
All tests successful.
  1. run on current master
> git checkout fbe26424370f946c0895602ea0df138f67551e6c

> prove -I./lib ./t
t/00-load.t .................. ok   
t/basics_without_overload.t .. 1/? 
#   Failed test 'KEYs + PARENT + ANYWHERE'
#   at t/basics_without_overload.t line 53.
# Comparing $data as a Bag
# Missing: 'affe', 'zomtec', 1 reference

...

Test Summary Report
-------------------
t/basics_without_overload.t (Wstat: 16640 Tests: 159 Failed: 65)
  Failed tests:  8-9, 16, 23-27, 33-39, 43-50, 52, 54, 57
                60, 62-73, 76, 78, 80, 82, 86, 93-98, 125-126
                128, 131-133, 136-139, 141, 145, 154, 156-157
  TODO passed:   159
  Non-zero exit status: 65
t/data_dpath.t             (Wstat: 17408 Tests: 174 Failed: 68)
  Failed tests:  9-11, 17, 22, 30, 37-41, 47-53, 57-64, 66
                68, 71, 74, 76-87, 90, 92, 94, 96, 100
                107-109, 111-113, 140-141, 143, 146-148
                151-154, 156, 160, 169, 171-172
  TODO passed:   174
  Non-zero exit status: 68
t/regressions.t            (Wstat: 768 Tests: 6 Failed: 3)
  Failed tests:  2, 4, 6
  Non-zero exit status: 3
t/zeros.t                  (Wstat: 256 Tests: 10 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1

You should revert PR #16 or fix tests by PR #21 on master and merge this PR

Thanks!

@Madskill
Copy link
Contributor Author

@renormalist ping

@renormalist
Copy link
Owner

pong

I saw you created more pull requests, thanks!
I still need to review and think about them.

The other pull requests seem t orevert some of this one here.
Is this here still valid?

Thanks for your contributions and sorry for my indecisiveness on the topic - it's really just because I fear that it might break other user's dpaths...

@Madskill
Copy link
Contributor Author

Hi!

The code in master already incorrect after merged PR!..

  1. You should revert PR or fix tests by my PR on master
  2. Merge this PR
  3. Merge PR
  4. Check that tests is ok

@renormalist
Copy link
Owner

All, I am absolutely unsure if this doesn't break stuff, as I just can't get my head around the escape/quoting rules in Perl strings. So whenever your Data::DPath usage broke after v0.57 then please try to revert this merge here to and see if that's triggering your problem.

I'm trusting @Madskill now and apply this fix and test. Sorry for the superlong delay.

@renormalist renormalist merged commit 15717f1 into renormalist:master Aug 28, 2019
@renormalist
Copy link
Owner

Your contributed fixed quote escaping should be in the just released v0.58. Sorry again for that long pause, it's a shame that I was so hesitant.

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