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(bundler): allow downgrades, add option to disallow on Windows #3777

Merged
merged 5 commits into from
Mar 28, 2022

Conversation

lucasfernog
Copy link
Member

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@lucasfernog lucasfernog requested review from a team March 25, 2022 14:04
@lucasfernog lucasfernog requested a review from a team as a code owner March 25, 2022 14:04
@JonasKruckenberg
Copy link
Contributor

Sorry if I'm not up to speed on this one, but why do we need to change the default behavior? Making downgrades optional would also simplify the naming :) disallow_downgrades -> allow_downgrades and make it much easier to understand

@lucasfernog
Copy link
Member Author

I feel most users would want to allow downgrades. And it's also the behavior on Linux and macOS too (AFAIK).

@FabianLars
Copy link
Member

FabianLars commented Mar 25, 2022

as far as i am concerned it's because a) we allow downgrades on macos and linux and b) it's like the main "selling point" of the server variant of the updater.

Edit: argh, too slow :))

@JonasKruckenberg
Copy link
Contributor

hmmmm yeah that makes sense. What about the name though? disallow_downgrades is a nice alliteration but it's also a mouthful 😅

@amrbashir
Copy link
Member

I think "allow_downgrades" with default value of true is better naming.

@lucasfernog
Copy link
Member Author

Changed it @amrbashir @JonasKruckenberg

amrbashir
amrbashir previously approved these changes Mar 26, 2022
/// For instance, if `1.2.1` is installed, the user won't be able to install app version `1.2.0` or `1.1.5`.
///
/// The default value of this flag is `true`.
#[serde(default)]
Copy link
Contributor

Choose a reason for hiding this comment

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

But isn't serde[default] for bools false?

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a problem with configuration flags that defaults to true :( way easier if it defaults to false

@lucasfernog lucasfernog merged commit 8b807e0 into dev Mar 28, 2022
@lucasfernog lucasfernog deleted the refactor/windows-downgrades branch March 28, 2022 00:34
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.

None yet

4 participants