-
Notifications
You must be signed in to change notification settings - Fork 139
Update module directories to be within their focus directory #58
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsilverstein This looks good except for the logic change for parsing modules, which is now coupled to which focus areas we have defined and also causes tests to fail. Can you update that? I believe some related tests need to be updated as well.
Other than that, only minor comments for e.g. a few oversights that still need to be updated.
|
@adamsilverstein Also, please add the appropriate PR labels and milestone based on #52. |
|
Thanks for the feedback @felixarntz - I'll work on addressing these points. |
…eas`" This reverts commit 9ecb11a. # Conflicts: # admin/load.php
|
Addressed feedback, updating tests next to work with new setup. |
d3b818a to
3c4681a
Compare
|
@felixarntz ready for review:
|
felixarntz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsilverstein Almost lgtm now! Only a few last minor comments.
|
@adamsilverstein Note there is also #60 being implemented right now. Depending on which of these two PRs is merged first, the other one may need an update. |
|
@adamsilverstein I've pushed two small commits here just to rebase and adjust the code from #60 to work with this, since that was just merged (see #58 (comment)). |
Co-authored-by: Felix Arntz <[email protected]>
Co-authored-by: Felix Arntz <[email protected]>
|
@felixarntz - ready for another review when you have a chance |
felixarntz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsilverstein Excellent work!
Addresses #46
webp-uploadsmodule into aimagesfolder inmodules.perflab_get_modulesto parse through all the focus folders to look for modules.webp-uploadschanges toimages/webp-uploads- for this reason, existing users will need to re-enable the webp-uploads module. Not a big concern since only developers are testing this so far.Focus:from the headers of all sample code and from thewebp-uploadsmodule header (only change to module itself)perflab_get_module_datato use a regex to extract the module type from the module filename (which includes the focus slug as part of its path). Feedback on regex always welcome!