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

small fix for unstable library feature #1468

Merged
merged 1 commit into from Aug 8, 2022
Merged

small fix for unstable library feature #1468

merged 1 commit into from Aug 8, 2022

Conversation

Ludmuterol
Copy link
Contributor

What kind of change does this PR introduce?

  • Fix

Did this PR introduce a breaking change?

  • I honestly don't know. but i don't think so.

Explanation:

I found when i was compiling the v0.9.0 on Windows it says:

error[E0658]: use of unstable library feature 'bool_to_option'
  --> src\settings\window_geometry.rs:89:26
   |
89 |                         .then_some(grid_size)
   |                          ^^^^^^^^^
   |
   = note: see issue #80967 <https://github.com/rust-lang/rust/issues/80967> for more information

error[E0658]: use of unstable library feature 'bool_to_option'
  --> src\settings\window_geometry.rs:96:26
   |
96 |                         .then_some(position)
   |                          ^^^^^^^^^
   |
   = note: see issue #80967 <https://github.com/rust-lang/rust/issues/80967> for more information

For more information about this error, try `rustc --explain E0658`.
error: could not compile `neovide` due to 2 previous errors

and in the mentioned Github issue there is this message: rust-lang/rust#80967 (comment)
which states:
"Every use of the then_some function can be trivially translated to use then instead, by adding || in front of the argument"
the following messages state some situations where this fix isn't this easy but in this case i could just change to the then function.

@MultisampledNight
Copy link
Contributor

Thank you! The linked comment is from January 2021 though, and the feature has been stabilized on the May 5 since. It's now available in Rust version 1.62 released on June 30, which is the latest at time of writing. You just need to run rustup update to receive the latest version, assumed you installed Rust through rustup.

Then, the change is indeed useful to allow older distro-packaged Rust versions to work. So I'd just merge this (after waiting for the CI).

By the way, it's more comfortable for reviewing if you commit patches to be submitted to a PR to a different branch than main on your fork, but that's not important.

@MultisampledNight MultisampledNight merged commit 6dd18e9 into neovide:main Aug 8, 2022
@Ludmuterol
Copy link
Contributor Author

:) Thanks for the feedback!
Just for clarification: I fork the project and create stuff on a different branch on my forked project. and then i PR that branch directly to this main branch? or do i PR with creating a new branch here? What is more comfortable when i do it on a different branch?

@MultisampledNight
Copy link
Contributor

No worries! Happy to do so, provided it helps you of course.

It's not about which branch you PR to — it's about which branch you PR from. In a typical testing setup, a person has only one repository cloned only and uses different remotes for looking at pull requests. For example, to add your (past) fork as a remote, this would be the theoretical shell session for it:

 $ git remote add ludmuterol git@github.com:Ludmuterol/neovide.git
 $ git remote update ludmuterol
...
 $ git remote show
origin  # this repository here
ludmuterol  # your fork

After the add and update, git is now aware of all branches your fork has. So, if I'd like to switch to a fictional branch only on your fork called old-rust-fix, I could just use

 $ git switch old-rust-fix
Switched to branch 'old-rust-fix'
Your branch is up to date with 'ludmuterol/old-rust-fix'.

...and now I could test your theoretical changes, change files, commit, push and it all would show up in your fork, just like I'd work on this repository.

But the situation is a bit more funny when the changes on your fork are in a branch called main. Because then, if I try to switch to your main while being on the one of this repository already, git just gives me

 $ git switch main
Already on 'main'
Your branch is up to date with 'origin/main'.

Which requires me to use git checkout instead:

 $ git checkout -b ludmuterols-main --track ludmuterol/main
branch 'ludmuterols-main' set up to track 'ludmuterol/main'.
Switched to a new branch 'ludmuterols-main'

Which requires me to push in a special way to avoid mistakes:

 $ git push
fatal: The upstream branch of your current branch does not match
the name of your current branch.  To push to the upstream branch
on the remote, use

    git push ludmuterol HEAD:main

To push to the branch of the same name on the remote, use

    git push ludmuterol HEAD
...
 $ git push ludmuterol HEAD:main
...

Additionally, if you'd suddenly decide to prepare a second patch at the same time, you'd need to somehow differentiate between them. Branches are perfect for that, if you'd also include the second patch on the main branch, things would be quite a mess.

In the end, it's not really that much of a change, it just makes some commands more verbose and makes compartmentalization more difficult.

@Ludmuterol
Copy link
Contributor Author

I see 😊 thank you very much for the explanation!

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

2 participants