Skip to content

Comments

Improve Windows coverage and fix Windows tests#1176

Merged
rocky merged 12 commits intomasterfrom
windows-compatability
Mar 9, 2021
Merged

Improve Windows coverage and fix Windows tests#1176
rocky merged 12 commits intomasterfrom
windows-compatability

Conversation

@rocky
Copy link
Member

@rocky rocky commented Mar 7, 2021

files.py: implement OperatingSystem option

@rocky rocky requested a review from mmatera March 7, 2021 21:48
@rocky rocky force-pushed the windows-compatability branch from e182236 to a988068 Compare March 7, 2021 21:50
files.py: implement OperatingSystem option
@rocky rocky force-pushed the windows-compatability branch from a988068 to c63e46f Compare March 7, 2021 21:52
@rocky rocky marked this pull request as draft March 7, 2021 21:54
@rocky
Copy link
Member Author

rocky commented Mar 7, 2021

@mmatera there are some fixes in here that might be split ou

In mathics/Mathics/mathics/builtin/importexport.py there were dictionaries with duplicate keys.
If you use a linter like pyflakes this will catch this kind of error.

Also, the black formatter (pip install black) can assist in finding mistakes like this and it would be very good to use when you have huge lists like this.

I may try to get actions CI working on this later.

@mmatera
Copy link
Contributor

mmatera commented Mar 7, 2021

By now, looks very good!

Also, the black formatter (pip install black) can assist in finding mistakes like this and it would be very good to use when you have huge lists like this.

Indeed, today I installed pyflakes, and I found this issue. Thanks for solving it!

@mmatera
Copy link
Contributor

mmatera commented Mar 7, 2021

Also, maybe we can merge this branch with #1173 with has some similar contributions.

files.py: implement OperatingSystem option
@rocky rocky force-pushed the windows-compatability branch from c63e46f to 3d1815d Compare March 8, 2021 00:17
Copy link
Contributor

@mmatera mmatera left a comment

Choose a reason for hiding this comment

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

LGTM


>> ToFileName[{"dir1", "dir2", "dir3"}]
= dir1/dir2/dir3
= dir1...dir2...dir3
Copy link
Contributor

Choose a reason for hiding this comment

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

... would be also a good way to deal with digits beyond accuracy in numeric tests!

evaluation.timeout_queue.pop()
return failexpr.evaluate(evaluation)
except:
if sys.platform != "win32":
Copy link
Contributor

Choose a reason for hiding this comment

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

Is TimeConstrained what is failing? or is the expression we have chosen to test it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the implementation is Unix/POSIX specific. I haven't investigated though.

@rocky rocky force-pushed the windows-compatability branch from 44c3901 to 6c0a8cc Compare March 9, 2021 13:53
rocky added 3 commits March 9, 2021 12:49
importexport.py: reinstate some of the changes made before lost due to conflitcs
CHANGES.rst: note what we did here
lists.py: remove warning due to bad escape character in \ldots
@rocky rocky force-pushed the windows-compatability branch from cd9149e to 940ead7 Compare March 9, 2021 18:36
@rocky rocky force-pushed the windows-compatability branch from 940ead7 to 0e87781 Compare March 9, 2021 18:44
@rocky rocky marked this pull request as ready for review March 9, 2021 18:55
@rocky
Copy link
Member Author

rocky commented Mar 9, 2021

@mmatera this has been most not fun to work on. I have spent way too much time on this, so I am stopping work on this.

Also I am merging this in because coping with the conflicts with master, in particular bultins/importexport were also a big chore. I resolved these mostly in favor of master at the expense of losing some fixes here.

In general, it appears that doc tests should not rely on unicode output such as were found in Base64Encode and Print. Instead put those under pytest and test for character-set capability.

In sum although this is incomplete, it is a start.

@rocky rocky merged commit 440afcf into master Mar 9, 2021
@mmatera
Copy link
Contributor

mmatera commented Mar 9, 2021

@rocky, thanks for all this work! I will think about how to solve the code page issue in the docstrings.

@rocky
Copy link
Member Author

rocky commented Mar 9, 2021

@mmatera It is not about the docstrings per se. It has to do with test output that uses unicode.

At some point we will probably have a better way to indicate unicode and/or native character set capabilities.

However setting attributes like this is doomed for the docstring system as it is which needs to be replaced by something that is not so custom.

However easy it is to add docstring tests, please keep in mind:

  • they need to work basically in all environments
  • they should show examples of things that a user might be interested in

Until we use some other less custom system, if a test doesn't fall into this category consider putting it in pytest.

@mmatera
Copy link
Contributor

mmatera commented Mar 9, 2021

@rocky,completely agree with that criteria. However, I think that it is possible to implement tests using unicode / the character table of the platform. In any case, I think that your work on improving compatibility improves a lot the quality of the code that we have in mathics.

@mmatera mmatera deleted the windows-compatability branch March 14, 2021 18:27
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