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

Add squeeze() to dynamic dimension arrays #1396

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

barakugav
Copy link
Contributor

No description provided.

@barakugav barakugav mentioned this pull request Jul 3, 2024
Copy link
Collaborator

@nilgoyette nilgoyette left a comment

Choose a reason for hiding this comment

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

Moreover, can you please merge master in your branch?

src/impl_dyn.rs Show resolved Hide resolved
@barakugav
Copy link
Contributor Author

Moreover, can you please merge master in your branch?

Rebased

@nilgoyette
Copy link
Collaborator

Thank you for your contribution.

@nilgoyette nilgoyette enabled auto-merge July 9, 2024 00:47
@nilgoyette nilgoyette disabled auto-merge July 9, 2024 00:57
@nilgoyette
Copy link
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...?

@barakugav
Copy link
Contributor Author

@nilgoyette can we merge this PR?

@bluss
Copy link
Member

bluss commented Jul 21, 2024

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

@nilgoyette
Copy link
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
Copy link
Member

bluss commented Jul 21, 2024

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.

@nilgoyette
Copy link
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
Copy link
Member

bluss commented Jul 22, 2024

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?).

@nilgoyette
Copy link
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.

@nilgoyette
Copy link
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
Copy link
Member

bluss commented Jul 23, 2024

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
Copy link
Member

bluss commented Jul 28, 2024

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

@bluss bluss added this to the 0.16.0 milestone Aug 1, 2024
@bluss bluss changed the title Adds squeeze() to dynamic arrays Add squeeze() to dynamic dimension arrays Aug 2, 2024
@bluss bluss enabled auto-merge August 2, 2024 19:23
@bluss 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
11 checks passed
@barakugav barakugav deleted the squeeze branch August 6, 2024 12:29
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