M2 icon indicating copy to clipboard operation
M2 copied to clipboard

CellularResolutions.m2 pull request for JSAG (try 2)

Open jkyang92 opened this issue 6 months ago • 6 comments

This is the pull request for the CellularResolutions package for JSAG. The changes are only the author emails since the package was already previously included in the Macualay2 distribution.

See also, these previous pull requests: https://github.com/Macaulay2/M2/pull/2833, https://github.com/Macaulay2/M2/pull/2844

I've created a draft pull request this time so it won't get merged by accident. See #3968

CC: @OlaSobieska

jkyang92 avatar Sep 01 '25 22:09 jkyang92

Thanks, @jkyang92 (and @OlaSobieska) ! I'm pretty sure your package is in good shape, but let's see what comments we would collect in this PR --- this would be a good test case.

I will propose to have an M2Internals discussion of how to review a general JSAG-linked PR in various scenarios (new package, existing package, etc.) and possible outcomes (for the PR and for the JSAG submission).

antonleykin avatar Sep 02 '25 02:09 antonleykin

but let's see what comments we would collect in this PR --- this would be a good test case.

As I mentioned here, GitHub doesn't allow comments on lines not changed in given PR, nor does it even show beyond a few lines surrounding the diff (e.g. I can see that this package has AuxiliaryFiles => true but only the main file is visible from this PR). Doesn't this make using GitHub PR review interface essentially pointless here?

An alternative is to also leave comments on the previously listed pull requests, but even then it's hard to leave a comment on v1 and check that is hasn't already been changed in v2, etc.

mahrud avatar Sep 02 '25 03:09 mahrud

Doesn't this make using GitHub PR review interface essentially pointless here?

No, there are still plenty of options for constructive feedback.

antonleykin avatar Sep 02 '25 17:09 antonleykin

No, there are still plenty of options for constructive feedback.

Do you mean just commenting as if this was an issue? Could you give an example of giving feedback using the PR review interface here?

mahrud avatar Sep 02 '25 18:09 mahrud

No, there are still plenty of options for constructive feedback.

Do you mean just commenting as if this was an issue? Could you give an example of giving feedback using the PR review interface here?

Please (from now) comment only on the package associated to this PR here. I've created a topic on Zulip (M2Internals Discussions) to collect comments on the general JSAG M2 PR process.

Request: make your comments short and to the point and moderate/neutral in tone.

antonleykin avatar Sep 03 '25 18:09 antonleykin

The code looks quite clean, but as we know every program can be made one line shorter. Perhaps this is the line: https://github.com/jkyang92/M2/blob/cellres-jsag/M2%2FMacaulay2%2Fpackages%2FCellularResolutions.m2#L357

antonleykin avatar Sep 08 '25 00:09 antonleykin