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

[getattrdeep-] remove unnecessary check #1927

Closed
wants to merge 1 commit into from

Conversation

midichef
Copy link
Contributor

@midichef midichef commented Jun 20, 2023

In 6b8deca, I saw this note in the log

  • this will make every ItemColumn/AttrColumn slower :(

That slower check has since become unnecessary, as the problem it was attempting to work around, was later fixed directly by
bfdfeb3 .
(details of how that worked are here #1696 (comment) )

I've been running Visidata without the check for for a few months. It seems fine. (Though I tested it under python 3.10, not 3.8/3.9 where the recursion errors were seen). The :( can be changed to a :)

@anjakefala
Copy link
Collaborator

(@midichef the link to the commit for the recursion crash fix was 404ing. I attempted to update the link. Is that the accurate commit you were referring to?)

@anjakefala
Copy link
Collaborator

Hi @midichef! =)

The tests seem to be legitimately failing: with the expected "golden output" not matching.

I will be able to allocate some time to investigate why later, but you could probably get a headstart. Do you need any guidance for understanding how the tests run or work?

@midichef
Copy link
Contributor Author

midichef commented Jun 22, 2023

I was able to figure out tests thanks to docs/test.md. I'll take a look at what's going on.

@midichef
Copy link
Contributor Author

I wasn't able to reproduce this error on 3.7. I'll close this for now and reopen when I can explore it.

@midichef midichef closed this Jul 19, 2023
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.

None yet

2 participants