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

Refactor into get_file_paths_and_names() function #81

Merged

Conversation

BrianHenryIE
Copy link
Member

@BrianHenryIE BrianHenryIE commented Sep 20, 2023

Adds a function/moves the logic for determining the source directory, the output directory, the output filename, and the plugin directory name the archive will extract to.
Which required moving the logic to determine the plugin version into its own function too (less attention paid here).

No change in behavior indicated through the tests. This will hopefully address #76 but I do not have a Windows setup to test it on. See the added regex on line 205 which now accounts for Windows absolute paths.

Also an opportunity for some code review as suggested yesterday.

@BrianHenryIE BrianHenryIE requested a review from a team as a code owner September 20, 2023 19:24
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Let us know when you're ready for a review on this!

@BrianHenryIE
Copy link
Member Author

@danielbachhuber Yes, please. I've further changes planned – in particular to make things faster. Currently, even when /vendor is in the .distignore, it's comparing every file inside vendor against the rules!

@swissspidy
Copy link
Member

Functionality-wise it looks good to me so far 👍 Just merged the latest changes so that tests pass again, which is a good sign too.

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Thanks again for your work on this, @BrianHenryIE !

@danielbachhuber danielbachhuber merged commit 73d568d into wp-cli:main Dec 6, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants