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

feat: integrating plugins and scripts #92

Merged
merged 4 commits into from
Sep 5, 2021

Conversation

mitosagi
Copy link
Collaborator

@mitosagi mitosagi commented Sep 4, 2021

Description

Remove code related to the script. This will require integration of plugins_list.xml with scripts_list.xml.

BREAKING CHANGE

  • Data about the scripts is deleted.
  • Change the file name from plugins_list.xml to packages_list.xml. The new tag names are <packages> and <package>.

Related Issue

fix #70

Motivation and Context

How Has This Been Tested?

  • Using the plugins_list.xmlpackages_list.xml attached below, you can install and uninstall the plugin and script without any problems.

plugins_list.zip
packages_list.zip

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@hal-shu-sato
Copy link
Member

In relation to this PR, I would like to call the plugin and the script together as a package, what do you think?

@mitosagi
Copy link
Collaborator Author

mitosagi commented Sep 5, 2021

I think calling it a package in the code or xml is a good idea.

The exception is that I think many people who don't do programming are not familiar with calling a zip file a package, so I'd like to call it plugin&script on the UI.

Do you think this is a good way to go?

@hal-shu-sato
Copy link
Member

Yes, I think so too.

Also, after all, apm is dealing with scripts as well as plugins, so I'm starting to think that the name of the application should be AviUtl Package Manager. (As in #90)
What do you think about it?

@mitosagi
Copy link
Collaborator Author

mitosagi commented Sep 5, 2021

I prefer the "plugin manager", which is easier to understand the features. But the "package manager" is also good.

@hal-shu-sato
Copy link
Member

I see...
But I'm afraid that "plugin" means that apm only deals with plugins, not scripts.
That bothers me.

BREAKING CHANGE: changing the file name and tag name of the xml file
@mitosagi
Copy link
Collaborator Author

mitosagi commented Sep 5, 2021

I replaced "plugin" with "package" in the code. I've changed the XML tag names, so I' ll send a PR to apm-data as well after the merge.

@hal-shu-sato hal-shu-sato self-requested a review September 5, 2021 07:01
@hal-shu-sato hal-shu-sato self-assigned this Sep 5, 2021
@hal-shu-sato hal-shu-sato added Feedback: enhancement New feature or request Type: improvement Improvements to program Type: new feature Suggesting new features Type: refactoring Refactoring codes labels Sep 5, 2021
@mitosagi
Copy link
Collaborator Author

mitosagi commented Sep 5, 2021

Certainly, it would be better to use a name that covers the function, so that we won't regret it later.

@hal-shu-sato
Copy link
Member

Certainly...
I'll send an issue later.

Copy link
Member

@hal-shu-sato hal-shu-sato left a comment

Choose a reason for hiding this comment

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

There is a bug.
Please check it.
But thank you for making this massive change.

@@ -168,6 +186,7 @@ module.exports = {
.filter((i) => i.isFile() && regex.test(i.name))
.map((i) => i.name);
return readdir(instPath).concat(
readdir(path.join(instPath, 'packages')).map((i) => 'packages/' + i),
Copy link
Member

Choose a reason for hiding this comment

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

There is a bug.
I don't have enough time to suggest all the fixes, so I apologize for the messy suggestions.

Suggested change
readdir(path.join(instPath, 'packages')).map((i) => 'packages/' + i),
readdir(path.join(instPath, 'plugins')).map((i) => 'plugins/' + i),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I certainly didn't check there.

I checked the other strings, but I think the only part I need to change is where you suggested.

Copy link
Member

@hal-shu-sato hal-shu-sato left a comment

Choose a reason for hiding this comment

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

Thank you for your commit!!

@hal-shu-sato hal-shu-sato merged commit a6ac1f3 into team-apm:main Sep 5, 2021
@mitosagi mitosagi deleted the refactor-plugin-script branch September 6, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback: enhancement New feature or request Type: improvement Improvements to program Type: new feature Suggesting new features Type: refactoring Refactoring codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring plugin.js and scripts.js as a whole
2 participants