Skip to content

Conversation

@DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Oct 30, 2025

Summary

  • Move CIF parsing logics from __init__ of CifParser to from_str
  • Revert accidentally removed filename as TextIOWrapper support (though this may not be intended) in Improve type annotations and comments for io.cif #3820, add a deprecation warning reverted, guess this was not intended, also doesn't agree with type/docstring
  • Currently the filename of CifParser constructor could also be the string for the CIF file itself (StringIO), it's a bit confusing (cc @ka-sarthak) but I guess this was intended to support from_str. After this refactoring this could be deprecated I guess

@DanielYang59 DanielYang59 force-pushed the deprecate-string-support-from-cifparser branch 5 times, most recently from 0886cdc to 57a62f1 Compare October 30, 2025 12:29
@ka-sarthak
Copy link

Thanks, @DanielYang59, for taking this up. My initial confusion was whether using a file handler is supported in the filename parameter, like:

cif = 'my_path.cif'
with open(cif) as file:
    parser = CifParser(filename=file) 

# output: "TypeError: Unsupported file format."

I think this used to work at some point, even though it might not have been intended. It's more of my negligence to misinterpret the interface.

I like that you make the error statement explicit now, as Unsupported file format alone is confusing, given that the file format (.cif) is correct.

@DanielYang59 DanielYang59 force-pushed the deprecate-string-support-from-cifparser branch 4 times, most recently from b29ac07 to 6ca7022 Compare October 30, 2025 17:41
@DanielYang59 DanielYang59 force-pushed the deprecate-string-support-from-cifparser branch from 6ca7022 to 0b70f4a Compare October 30, 2025 17:48
@DanielYang59 DanielYang59 force-pushed the deprecate-string-support-from-cifparser branch from 0b70f4a to 2cdb14d Compare October 30, 2025 17:49
@DanielYang59 DanielYang59 changed the title Deprecate CifParser init from str with filename Deprecate CifParser init support of filename as StringIO Oct 30, 2025
@DanielYang59 DanielYang59 marked this pull request as ready for review October 31, 2025 08:37
@shyuep shyuep merged commit 7134328 into materialsproject:master Oct 31, 2025
44 checks passed
@DanielYang59 DanielYang59 deleted the deprecate-string-support-from-cifparser branch October 31, 2025 16:03
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