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.

Sign up for GitHub

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

Jump to bottom

Problems with the new Diff::LCS::Change#to_ary implementation #48

Closed
halostatue opened this issue Jan 27, 2019 · 3 comments
Closed

Problems with the new Diff::LCS::Change#to_ary implementation #48

halostatue opened this issue Jan 27, 2019 · 3 comments
Assignees
@halostatue
Labels
Bug

Comments

@halostatue
Copy link
Owner

@knu: Unfortunately, there has been a bug in the diff-lcs test suite on Travis for a while (the test script is running bundle exec rake travis which is running the test task; something changed to where that was no longer dependent on the spec task), so the successful tests haven’t actually meant anything. I was prepping your merged PR #47 and could not get any of the tests to pass with bundle exec rake for various reasons.

The tests are correctly running now, but now we are seeing 88/274 failures. I suspect that this means we are going to need to revert #47, but I wanted to see if you had any suggestions before I do this.
@halostatue halostatue added the Bug label Jan 27, 2019
@halostatue halostatue self-assigned this Jan 27, 2019
@halostatue
Copy link
Owner Author

Here is a build that shows this: https://travis-ci.org/halostatue/diff-lcs/builds/484918724

@knu
Copy link
Contributor

knu commented Jan 28, 2019

As I see it, this library internally uses flatten() without specifying an exact level on a changeset and even tells in the documentation you could do that, so adding to_ary breaks itself and existing code. Sorry for the trouble!

@halostatue
Copy link
Owner Author

No problem. I’d like to figure out how to get the change you’ve made in, because I think it makes a lot of sense.

halostatue added a commit that referenced this issue Jan 28, 2019
@halostatue
Reintroduce Diff::LCS::Change#to_ary
03977e5 
-   This required some level of code remediation in the main library, but all
    tests now pass:

    -  Patchsets are now internally flattened one level explicitly, rather than
       using Array#flatten. This ensures that only the outer patchset array is
       flattened.

Fixes #48.
@halostatue halostatue mentioned this issue Jan 28, 2019
Merged
halostatue added a commit that referenced this issue Jan 28, 2019
@halostatue
Reintroduce Diff::LCS::Change#to_ary
efafb8c 
-   This required some level of code remediation in the main library, but all
    tests now pass:

    -  Patchsets are now internally flattened one level explicitly, rather than
       using Array#flatten. This ensures that only the outer patchset array is
       flattened.

Fixes #48.
halostatue added a commit that referenced this issue Feb 2, 2019
@halostatue
Reintroduce Diff::LCS::Change#to_ary
a798f6f 
-   This required some level of code remediation in the main library, but all
    tests now pass:

    -  Patchsets are now internally flattened one level explicitly, rather than
       using Array#flatten. This ensures that only the outer patchset array is
       flattened.

Fixes #48.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@halostatue halostatue

Labels
Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@knu @halostatue