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

Replace Version with semver lib #44

Merged
merged 1 commit into from
Jun 25, 2015
Merged

Replace Version with semver lib #44

merged 1 commit into from
Jun 25, 2015

Conversation

lightglitch
Copy link
Contributor

First Step to #32

@decebals
Copy link
Member

decebals commented Jun 5, 2015

My philosophy (see my projects, pippo for example) is to keep the number of dependencies as low as possible, because people/users want this. I add a new dependency only if that library brings value in project.
I think that Version is a decent, uncompleted implementation of semver. If you see some problems in this class please tell me.

@lightglitch
Copy link
Contributor Author

I understand that and the value that the lib brings is the support for semver ranges (https://github.com/zafarkhaja/jsemver#semver-expressions-api-ranges) that I'm going to need for compatibility check of plugins.

@decebals
Copy link
Member

decebals commented Jun 5, 2015

Thanks for your explanation. Can you comment on how you see #32 implementation to have a common understanding?

@lightglitch
Copy link
Contributor Author

I still thinking about it because I want to use the process information of the graph to do the validations.

But as a big picture I just going to put the plugin with the disable state if some of his dependencies are disabled. This with some error reporting information is enough for me.

@decebals
Copy link
Member

@gitblit I think that it's a good idea to have range validation on plugin version and I think that the jsemver library is a solid choice (no dependency, active development - I don't wish to implement a complex semver in pf4j). Is it ok for you this change?

@gitblit
Copy link
Contributor

gitblit commented Jun 25, 2015

It's ok with me.

decebals added a commit that referenced this pull request Jun 25, 2015
Replace Version with semver lib
@decebals decebals merged commit bab0c16 into pf4j:master Jun 25, 2015
private String pluginVersionSupport;

public PluginDependency(String dependency) {
int index = dependency.indexOf('@');
Copy link
Member

Choose a reason for hiding this comment

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

why @ and not :?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me the syntax plugin-at-version makes more sense but I saw that you were using : and forgot to change it.

Copy link
Member

Choose a reason for hiding this comment

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

No problem, we can keep @

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.

3 participants