Skip to content

BF: Load gifti parser iff parsing gifti files #405

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

Closed
wants to merge 2 commits into from

Conversation

effigies
Copy link
Member

Setting parser as a class attribute turns out not to be very necessary. Avoid the circular reference by calling the parser directly when needed.

Fixes #392.

@effigies effigies mentioned this pull request Feb 12, 2016
@effigies
Copy link
Member Author

Couldn't figure out a new test to write, since loading files is already tested. Could del GiftiImage.parser before trying to load, but this change removes the attribute altogether, so the test would be obsolete as soon as it's written.

@bcipolli
Copy link
Contributor

The intention here was to set a class-level property, similar to how header_klass is set on images. So now the corresponding parser class isn't easily discoverable for an image requiring one.

With that said, LGTM! This fixes a bug, and I don't see that having the parser class set explicitly on the class will matter anytime soon. Thanks @effigies !

@effigies
Copy link
Member Author

Definitely open to alternatives, but my only other idea was to just merge the two files together and eliminate the import problem.

I could add a comment, in case somebody decides to come back to this, if that would be helpful.

@bcipolli
Copy link
Contributor

Right. It's confusing. My solution wasn't the best either.

I think what you've done, with an informative comment, is the best choice available.

@bcipolli
Copy link
Contributor

("it's confusing" === "the best solution is hard to figure out")

@matthew-brett
Copy link
Member

Ouch - nasty - the Image class needs a Parser class and the Parser class needs the Image class.

I stared at it for a while - what do y'all think of this alternative?

master...matthew-brett:alternative-gifti-fix

@effigies
Copy link
Member Author

@matthew-brett 👍 Looks like a good alternative. No suggestions from my end. Feel free to close this PR if you go ahead with it.

@matthew-brett
Copy link
Member

Closed by #406

@effigies effigies deleted the gifti_parser_fix branch February 18, 2016 20:20
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.

3 participants