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

Rename CSV submodule #787

Closed
mgrover1 opened this issue Jan 16, 2024 · 8 comments · Fixed by #788
Closed

Rename CSV submodule #787

mgrover1 opened this issue Jan 16, 2024 · 8 comments · Fixed by #788
Assignees
Labels
bug Something isn't working

Comments

@mgrover1
Copy link
Collaborator

  • ACT version: 2.0
  • Python version: All
  • Operating System: All

Description

We should consider renaming the act.io.csv submodule in ACT. It will lead to import issues with pandas, as it is not recommended to have modules titled csv

see the thread here

We can still keep the same API - read_csv, but we should rename to something like "text" or "tabular". Thoughts?

What I Did

Upon submitting a PR, the following error appeared:

ImportError: cannot import name 'QUOTE_NONNUMERIC' from partially initialized module 'csv' (most likely due to a circular import) (/Users/jrobrien/dev/ACT/act/io/csv.py)
@mgrover1 mgrover1 added the bug Something isn't working label Jan 16, 2024
@zssherman
Copy link
Collaborator

@mgrover1 I would be fine with that, we would just need to reach out to the DQ folks etc. It should not change the import path if act.io.read_csv is used, but i know sometimes act.io.csv. has been used. So would have to make sure it doesn't break anything. I post in the act channel

@AdamTheisen
Copy link
Collaborator

I would be okay changing it to act.io.text since it really can do more than just csv

@mgrover1
Copy link
Collaborator Author

I agree here @AdamTheisen !! I can draft up a PR.

@mgrover1 mgrover1 self-assigned this Jan 16, 2024
@zssherman
Copy link
Collaborator

@AdamTheisen @mgrover1 Sounds good! I say yeah, submit the PR and we'll greenlight once we get the ok from the DQ folks

@mgrover1
Copy link
Collaborator Author

PySP2 will need an update as well here (cc @rcjackson )

@rcjackson
Copy link
Collaborator

PySP2 will need an update as well here (cc @rcjackson )

Just let me know what is decided for the new name then I can push a micro-release.

@mgrover1
Copy link
Collaborator Author

act.io.text is the decision based on the conversation in this thread

@zssherman
Copy link
Collaborator

zssherman commented Jan 16, 2024

@mgrover1 Can we drop the second import path/reference as we're importing read_csv in the init?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants