Skip to content
This repository has been archived by the owner on Jan 16, 2023. It is now read-only.

Fix issue 191 #192

Closed
wants to merge 76 commits into from
Closed

Fix issue 191 #192

wants to merge 76 commits into from

Conversation

thenewguy
Copy link
Contributor

No description provided.

@thenewguy
Copy link
Contributor Author

@antonagestam your linters are killing me =)

@thenewguy
added test ensuring strategy.post_copy_hook is called
30c19da
@thenewguy
added missing import
35de853
@thenewguy
removed types
62c0e10
@thenewguy
adjusted decorator order
22e32a3
@thenewguy
adjusted bases
9576843
@thenewguy
adjusted base class
456e0b8
@thenewguy
fixed missing init arg
0426d4b
@thenewguy
fixed typo
d807371
@thenewguy
adjusted base strategy from hash test
e1d5e2d
@thenewguy
let inheritance best me
e605067
@thenewguy
Revert "let inheritance best me"
058addd 
This reverts commit e605067.
@thenewguy
added missing method
34c5a00
@thenewguy
passed storage through
4202373
@thenewguy
set staticfiles storage override
cc132d7
@thenewguy
fixed assert name
d7e37a4
@thenewguy
set expected args
7aa71ed
@thenewguy
fixed reference to remote storage
0a1a2fa
@thenewguy
fixed reference
2781264
@thenewguy
set expected storage to mock.ANY
73891b0
@thenewguy
fixed expectation
8bc0bdd
@thenewguy
Revert "fixed expectation"
e597b0d 
This reverts commit 8bc0bdd.
@thenewguy
updated docstring
f1735c5
@thenewguy
called post_copy_hook on skip
3bb8875
@thenewguy
tested when collectstatic skips file copied==False
1a58f9f
@thenewguy
changed base class to strategy
14e700a
@thenewguy
Copy link
Contributor Author

thenewguy commented May 10, 2020 •
edited
Loading

Are you sold on the name post_copy_hook()? When it was called for both skipped and copied files it made the most sense but since the skipped hook is now copy_skipped_hook() I am thinking post_copy_hook() should be renamed to file_copied_hook()

And perhaps copy_skipped_hook() --> file_skipped_hook() to be the same

@thenewguy
Copy link
Contributor Author

Ok waiting for feedback. Otherwise RFC

antonagestam
antonagestam reviewed May 16, 2020
View reviewed changes
Copy link
Owner

@antonagestam antonagestam left a comment

Choose a reason for hiding this comment

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

I have another set of comments.

CHANGELOG.md Outdated Show resolved Hide resolved
collectfast/strategies/base.py Outdated Show resolved Hide resolved
collectfast/strategies/base.py Outdated Show resolved Hide resolved
collectfast/strategies/base.py Outdated Show resolved Hide resolved
collectfast/tests/command/test_copy_hooks.py Show resolved Hide resolved
collectfast/tests/command/test_copy_hooks.py Outdated Show resolved Hide resolved
collectfast/tests/strategies/test_caching_hash_strategy.py Outdated Show resolved Hide resolved
collectfast/tests/strategies/test_caching_hash_strategy.py Outdated Show resolved Hide resolved
@thenewguy
Copy link
Contributor Author

Suggestions applied with two questions inline with your comments. Ready for review

@thenewguy
Copy link
Contributor Author

All resolved - RFC

@thenewguy
Copy link
Contributor Author

@antonagestam Are you happy with this? Want to make sure this is resolved before moving on to the next issue. Really happy with my use of collectstatic so far! It is awesome

antonagestam pushed a commit that referenced this pull request Jun 5, 2020
@thenewguy @antonagestam
updated to call command twice directly per discussion
d02f493 
#192 (comment)
antonagestam pushed a commit that referenced this pull request Jun 5, 2020
@thenewguy @antonagestam
removed test_get_local_file_hash_memoization() per discussion
1fb43ed 
#192 (comment)
@antonagestam antonagestam mentioned this pull request Jun 5, 2020
Merged
@antonagestam
Copy link
Owner

I merged a cleaned up version of this in #198, thank you for working on this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@antonagestam antonagestam antonagestam left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@thenewguy @antonagestam