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

Remove dependencies to nightly build of rust compiler. #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bhavitsharma
Copy link

Remove dependencies on nightly build of rust compiler. #89

  1. vec_remove_item
  2. std::options::NoneError
    Also, I formatted it using rustfmt, hence the reason for large amount of diff.

@rabite0
Copy link
Owner

rabite0 commented May 24, 2020

Holy shit, that's awesome! Way to go!

But could you please remove the reformatting? It's not always a clear win. For example, I recently started aligning method calls like this:

    pub fn selected_file(&self) -> &File {
        let file = self.current_item
                       .as_ref()
                       .or_else(|| self.content.iter_files().nth(0))
                       .unwrap();
    }

I think that's more readable than what rustfmt does by default. There's a lot of custom-y formatting it removes that actually hurts readability. Maybe it can be configured in a way that keeps most of the aesthetics? I'll need to look into that. Depending on how much you're going to contribute in the future I'd be willing to compromise of course ;). In any case, it should be a separate commit...

@bhavitsharma
Copy link
Author

bhavitsharma commented May 24, 2020

maybe it can be configured in a way that keeps most of the aesthetics

Would you mind sharing the flags and custom settings of rust-fmt that you use so we can decide on a common environment?
Also, can you create a discord server so we can chat easily? It's the preferred method these days. Even LLVM, Rust servers are the official way to chat over stuff.

@rabite0
Copy link
Owner

rabite0 commented May 24, 2020

There are sadly still issues with rustfmt regarding method chains. Especially when let bindings are involved it just doesn't work as well as I'd like. So we're stuck with doing it manually and have to live with some inconsistency here, or there. Sometimes it's not even an accident, since readability is more important than consistent formatting IMO. What would be nice if there was a way to selectively enable some rustfmt stuff like import sorting, but I don't think there is...

06kellyjac added a commit to 06kellyjac/hunter that referenced this pull request Oct 6, 2020
This commit comprises changes made by @bhavidsharma in PR rabite0#91 without
the other formatting changes.
rabite0#91

It took quite a while to scroll through and pick out the important bits.

This commit wasn't written with Bhavit but I thought it important to
include them as a co-author as it was originally their changes.

Co-authored-by: Bhavit Sharma <bhavitsharma@google.com>
@06kellyjac 06kellyjac mentioned this pull request Oct 6, 2020
06kellyjac added a commit to 06kellyjac/hunter that referenced this pull request Oct 6, 2020
This commit comprises changes made by @bhavidsharma in PR rabite0#91 without
the other formatting changes.
rabite0#91

It took quite a while to scroll through and pick out the important bits.

This commit wasn't written with Bhavit but I thought it important to
include them as a co-author as it was originally their changes.

Co-authored-by: bhavidsharma <bhavitsharma@google.com>
06kellyjac added a commit to 06kellyjac/hunter that referenced this pull request Oct 6, 2020
This commit comprises changes made by @bhavidsharma in PR rabite0#91 without
the other formatting changes.
rabite0#91

It took quite a while to scroll through and pick out the important bits.

This commit wasn't written with Bhavit but I thought it important to
include them as a co-author as it was originally their changes.

Co-authored-by: Bhavit Sharma <bhavitsharma@google.com>
@bhavitsharma
Copy link
Author

bhavitsharma commented Jun 11, 2021

sorry everyone for this mess, I had other commitments at my work. I will try to work on it this weekend (without any rustfmt). I made these changes and had format-on-save option turned on.
Edit: I see someone already made a PR. Thanks!

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.

2 participants