Skip to content

Refactor resources module #1695

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

Conversation

matthewhegarty
Copy link
Contributor

Problem

resources.py has grown to be a large module. I have moved 'declarative' code and and ResourceOptions into separate modules.

There are no functional changes in this PR.

Acceptance Criteria

  • All existing tests pass

@matthewhegarty matthewhegarty changed the base branch from main to release-4 November 27, 2023 09:48
Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this but would throw out using a subfolder called resources and placing the files in there as an option that might be easier to understand.

@matthewhegarty
Copy link
Contributor Author

Hi Ryan

Thanks for reviewing, much appreciated.

I did think about using a subfolder. SQLAlchemy has a subfolder called ext that they put their declarative code into, however that name convention doesn't seem to apply here.

It does seem that since this code is helper code for resources.py it should be hidden away.

We could go with resources, but that then implies that resources.py should go in there as well, then presumably releated fields.py, instance_loaders.py etc, then we have a large change on our hands.

To keep things simple, I wonder if it is easier just to keep them in separate top level modules as in this PR. What do you think?

@pokken-magic
Copy link

Hi Ryan

Thanks for reviewing, much appreciated.

I did think about using a subfolder. SQLAlchemy has a subfolder called ext that they put their declarative code into, however that name convention doesn't seem to apply here.

It does seem that since this code is helper code for resources.py it should be hidden away.

We could go with resources, but that then implies that resources.py should go in there as well, then presumably releated fields.py, instance_loaders.py etc, then we have a large change on our hands.

To keep things simple, I wonder if it is easier just to keep them in separate top level modules as in this PR. What do you think?

Yeah I was thinking the django models folder style route where you make a resources folder, then import everything into the init.py in that folder would make it so you didn't have to change anything - everything still imports from resources.

But there're definitely issues with that approach :)

@matthewhegarty
Copy link
Contributor Author

Are you ok for me to merge as it is?

@pokken-magic
Copy link

Are you ok for me to merge as it is?

Yup, always something we can tackle later if desired and this is an improvement.

@matthewhegarty matthewhegarty merged commit 1ec6002 into django-import-export:release-4 Nov 27, 2023
@matthewhegarty matthewhegarty deleted the refactor-resources-module branch November 27, 2023 12:49
Viicos added a commit to Viicos/typeshed that referenced this pull request Jan 19, 2025
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.

2 participants