-
Notifications
You must be signed in to change notification settings - Fork 282
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
Adding semver compatibility check for Windows and Linux #502
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.
The implementation looks alright, but I am not sure I like the compatibility thing though, I think mismatched versions, even if its a patch version shouldn't launch a different version.
However, I think this maybe a desired behavior for some people so I'd suggest:
- Add a behavior enum
enum LaunchBehavior { Same, #[cfg(feature = "compatible")] Compatible }
- Add a builder for the plugin that will have a method to specify which behavior they need
- make the behavior that requires
semver
behind a feature flag just to not add unnecessary dependencies for default behavior.
does that sound alright?
Okay, I will try to add that when I have some time. |
Any update on this one ? |
I did not have time to implement the feature flag as it will probably take quite some time and I had lots of other things on my plate so far. For now I'm using my fork instead for my projects: https://github.com/matthme/plugins-workspace/tree/single-instance-semver maybe that's useful for you as well. |
@amrbashir would it be alright to have the feature flag itself change the behavior? |
I guess so yeah |
coolio. mainly asking cause i'm lazy 🙃 |
4dc4461
to
e8986e2
Compare
choo choo, i'm merging all v1 prs, get out of my way :P
Addressing #494 . This PR is assuming that this is desired default behavior. If it should rather be hidden behind a feature flag, I can try to add that.