Skip to content

typo + guava #40519

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

Merged
merged 10 commits into from
Aug 5, 2025
Merged

typo + guava #40519

merged 10 commits into from
Aug 5, 2025

Conversation

GMS103
Copy link
Member

@GMS103 GMS103 commented Jul 31, 2025

This tries to take into account #40484.

@enriqueartal: Could you test it?

⌛ Dependencies

Built on top of #40512.

@GMS103
Copy link
Member Author

GMS103 commented Jul 31, 2025

I was told nullptr is defined to be 0, so I did not replace the latter with the former.

@GMS103 GMS103 requested a review from enriqueartal July 31, 2025 20:38
@enriqueartal
Copy link
Contributor

enriqueartal commented Jul 31, 2025

Built in Fedora 42, the usual failed test for semigroups.rst. Dicriticals are much easier :)

@GMS103
Copy link
Member Author

GMS103 commented Jul 31, 2025

TL;DR Nothing new.

With
cmake_minimum_required(VERSION 3.25...4.0.3) (present situation)
SageMath version 10.7.beta9 + #40519 gives the following after
./configure --enable-gap_packages --enable-semigroups --enable-symengine

macOS 15.6
make succeeds.
make ptestlong gives 2 errors:

(1)

sage -t --long --warn-long 30.0 --random-seed=… src/doc/en/reference/spkg/semigroups.rst
**********************************************************************
File "src/doc/en/reference/spkg/semigroups.rst", line 15, in doc.en.reference.spkg.semigroups
Failed example:
    libgap.LoadPackage("semigroups")       # optional - semigroups
Expected:
    true
Got:
    #I  ReadPackage could not read <digraphs>/gap/doc.g
    <BLANKLINE>
    true
**********************************************************************

as indicated by DIma.
(2)

sage -t --long --warn-long 30.0 --random-seed=… src/sage/algebras/quantum_groups/quantum_group_gap.py  # 1 doctest failed

related to TikZ (in fact there are some other failed tests, but they pass standalone).

Debian 12
make succeds.
make ptestlong gives the same semigroups.rst error (in fact there are some other failed tests, but they pass standalone).

@dimpase
Copy link
Member

dimpase commented Jul 31, 2025

I was told nullptr is defined to be 0, so I did not replace the latter with the former.

not sure what you mean here, but it's important to stick to correct types (and in C++ using 0 in place of where nullptr should be by right might be an error - for the latter being 0 is an implementation-dependent artefact)

@GMS103
Copy link
Member Author

GMS103 commented Jul 31, 2025

I was told nullptr is defined to be 0, so I did not replace the latter with the former.

not sure what you mean here, but it's important to stick to correct types (and in C++ using 0 in place of where nullptr should be by right might be an error - for the latter being 0 is an implementation-dependent artefact)

There are several places where
{0, 0, 0, 0, 0}
appears. Only 2 of them were replaced by Enrique with
{nullptr, 0, nullptr, nullptr, nullptr}
so it would be nice if somebody who knows about it makes the suitable changes.

@tobiasdiez
Copy link
Contributor

Same remark as in #40512 (comment)

@GMS103
Copy link
Member Author

GMS103 commented Aug 1, 2025

Why is conftest.py deleted here? This seems to be a mistake.

Sorry, it is surely my fault, but I do not know how I did it.
Should be back now.

@GMS103
Copy link
Member Author

GMS103 commented Aug 1, 2025

Thanks, Dima.

@vbraun
Copy link
Member

vbraun commented Aug 2, 2025

merge conflict, please fix

@@ -54,7 +54,7 @@ the third option to "export" them will not make sense.
The legacy SageNB has been retired in Sage 9.1.
Please use the Jupyter notebook.
Old SageNB worksheets can be converted to Jupyter notebooks
using the `sage --notebook=export`_ command or to RST
using the `sage --notebook=export` command or to RST
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a bad change in my PR causing the merge conflict:

Suggested change
using the `sage --notebook=export` command or to RST
using the ``sage --notebook=export`` command or to RST

@GMS103
Copy link
Member Author

GMS103 commented Aug 2, 2025

merge conflict, please fix

Should be fixed now. Asking for positive review.

@vbraun: Please merge this instead of #40512.

@dimpase
Copy link
Member

dimpase commented Aug 2, 2025

thanks

Copy link

github-actions bot commented Aug 2, 2025

Documentation preview for this PR (built with commit e7f3ac9; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 3, 2025
This tries to take into account sagemath#40484.

@enriqueartal: Could you test it?

### ⌛ Dependencies

Built on top of sagemath#40512.

URL: sagemath#40519
Reported by: Guillermo Moreno-Socías
Reviewer(s): Dima Pasechnik, Enrique Manuel Artal Bartolo
@GMS103
Copy link
Member Author

GMS103 commented Aug 3, 2025

Thanks.

@vbraun vbraun merged commit bcdffce into sagemath:develop Aug 5, 2025
35 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants