Skip to content

Refactor resources module#1695

Merged
matthewhegarty merged 6 commits intodjango-import-export:release-4from
matthewhegarty:refactor-resources-module
Nov 27, 2023
Merged

Refactor resources module#1695
matthewhegarty merged 6 commits intodjango-import-export:release-4from
matthewhegarty:refactor-resources-module

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

@rpsands rpsands 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?

@rpsands
Copy link

rpsands commented Nov 27, 2023

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?

@rpsands
Copy link

rpsands commented Nov 27, 2023

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