FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add `squeeze()` to dynamic dimension arrays by barakugav · Pull Request #1396 · rust-ndarray/ndarray · GitHub

Add squeeze() to dynamic dimension arrays - #1396

Merged
bluss merged 1 commit into
rust-ndarray:masterfrom
barakugav:squeeze
Aug 2, 2024
Merged

Add squeeze() to dynamic dimension arrays#1396
bluss merged 1 commit into
rust-ndarray:masterfrom
barakugav:squeeze

Conversation

Copy link
Copy Markdown
Contributor

No description provided.

barakugav mentioned this pull request Jul 3, 2024

nilgoyette left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Moreover, can you please merge master in your branch?

Comment thread src/impl_dyn.rs

Copy link
Copy Markdown
Contributor Author

Moreover, can you please merge master in your branch?

Rebased

Comment thread src/impl_dyn.rs

Copy link
Copy Markdown
Collaborator

Thank you for your contribution.

nilgoyette enabled auto-merge July 9, 2024 00:47
nilgoyette disabled auto-merge July 9, 2024 00:57

Copy link
Copy Markdown
Collaborator

@adamreichold I don't have the right to merge this branch. There are several errors when I run cargo clippy --features docs on beta. Some of them are important and would necessitate a their own PR, like

warning: current MSRV (Minimum Supported Rust Version) is `1.57.0` but this item is stable since `1.80.0`
  --> src/data_repr.rs:75:27
   |
75 |         unsafe { self.ptr.add(self.len) }

There's no error when I use stable. I'm not sure why we run clippy on beta...?

Copy link
Copy Markdown
Contributor Author

@nilgoyette can we merge this PR?

bluss commented Jul 21, 2024

Copy link
Copy Markdown
Member

@nilgoyette that sounds like a false positive from clippy (we use a trait that adds that .sub method).

Copy link
Copy Markdown
Collaborator

@bluss Ok, thanks for the information. I can ask for an auto-merge but I get this message from github

Required statuses must pass before merging

so the only option I have is to click on Disable auto-merge. You probably have more options than me!

bluss commented Jul 21, 2024

Copy link
Copy Markdown
Member

Please fix rustfmt here if you can.
Then we need to fix clippy in a separate PR, rebase this one and merge. That's the sound way to inclusion.

Clippy doesn't always help us and here it's slowing us down instead. Stable or beta I don't know if it matters (what do you think?), but at least that means it's only a few times per Rust cycle that new lints get published, so it's a bit more predictable.

Copy link
Copy Markdown
Collaborator

Stable or beta I don't know if it matters (what do you think?), but at least that means it's only a few times per Rust cycle that new lints get published, so it's a bit more predictable.

Personally, I would never use beta or nightly on a mandatory cicd check (on a project that must run on stable). That's why I was asking above why you were doing it. I think it can only lead to bad surprises like what we have now. Since it doesn't bother you either ways, I'll create a MR to use stable for that check.

bluss commented Jul 22, 2024

Copy link
Copy Markdown
Member

I guess it's a little bit less likely to have surprises in clippy lints on stable, but I figure most of it will be the same between beta and stable (just a few weeks delayed?).

Copy link
Copy Markdown
Collaborator

Yes, you're right about the delay, but there's also another reason. When someone pushes a MR, he probably ran the tests and clippy before (I'm not the only one doing this?), and he's likely to be running stable. There's less chance of surprise if we run the cicd with the config that the contributor probably used.

Copy link
Copy Markdown
Collaborator

@bluss I'm not sure what I'm supposed to fix about the formatting. The three auto-format changes in this MR seem right to me. It's the CI that is wrong. Surely it's not nightly that produces this strange formatting?

bluss commented Jul 23, 2024

Copy link
Copy Markdown
Member

I was thinking @barakugav (the author) would fix rustfmt usually.

Ndarray uses nightly for rustfmt since we setup some formatting options, and they are only supported on nightly. #1375

bluss commented Jul 28, 2024

Copy link
Copy Markdown
Member

Can now be rebased on master because clippy errors are fixed there.

bluss added this to the 0.16.0 milestone Aug 1, 2024
bluss changed the title Adds squeeze() to dynamic arrays Add squeeze() to dynamic dimension arrays Aug 2, 2024
bluss enabled auto-merge August 2, 2024 19:23
bluss added this pull request to the merge queue Aug 2, 2024
Merged via the queue into rust-ndarray:master with commit 45009ff Aug 2, 2024
barakugav deleted the squeeze branch August 6, 2024 12:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL