search

paulfitz / cosmicos

Sending the lambda calculus into deep space
https://cosmicos.github.io/
GNU General Public License v2.0
135 stars 8 forks source link

Analysis: Clean up, argparse, and tests #33

Closed joha2 closed 2 years ago

joha2 commented 2 years ago
alanfwilliams commented 2 years ago

Great work here. Some questions:

  1. Is the meti message able to come in under GPL? I'm going to be honest and say I have no idea.
  2. Your script looks good. Once we have the travis ci working again, would you consider adding a test or two just to make sure everything works together properly in the future? I'm more than happy to do this myself.
paulfitz commented 2 years ago

For the meti message, I was going to suggest a link to archive.org, but can't find the message there - the dearet.org site is archived, but the message was in a google doc, and it doesn't seem to archive well. It is still available at https://drive.google.com/file/d/0B2jvn7fLH-rfOWNmMzJiODEtOTgzYi00ZjU0LTg5YTAtNDA4OTdlOTk2NmY5/view

I don't think we can include it here without permission of authors.

paulfitz commented 2 years ago

There is a version of the message in https://arxiv.org/abs/0911.3976 and some context in https://www.sciencedirect.com/science/article/abs/pii/S0265964611000415, but not seeing a great archived source for the message as it appeared on dearet.org.

paulfitz commented 2 years ago

Update: Michael Busch has material in dropbox, see https://twitter.com/michael_w_busch/status/1516920188935020544.

joha2 commented 2 years ago

@aw1231 I will add some tests. What kind of tests do you think are best suited? Should I add a test file which could be executed by CI? @paulfitz thank you very much for asking. I also read your discussion on Twitter. What I did not understand, yet, is: is the meti message allowed to stay in this PR or do I have to remove it? Since dropbox is not the ideal permanent storage, I would prefer to let it stay in the PR. But if this does not work, I'll remove it of course.

paulfitz commented 2 years ago

@joha2 you don't hold copyright on the meti message, and I haven't seen the copyright holders of the message grant any permissions for redistributing it. Material in this repo is supposed to be redistributable under the GPL, so adding the meti message isn't an option. I understand dropbox is not ideal permanent storage. It is unfortunate the message didn't make it into archive.org, which can legally make a copy (in the US) through an exemption for libraries and archives meeting certain conditions.

TLDR: please remove the message from the PR.

joha2 commented 2 years ago

@paulfitz thank you for clarifying this! I removed it.

The last point in this PR are the Python tests. What can be used best with the current CI/CD cycle? Is there a preferred solution? Where should the test suite go?

paulfitz commented 2 years ago

@joha2 tests are now run using the steps in https://github.com/paulfitz/cosmicos/blob/master/.github/workflows/main.yml - there's just a small smoke test in there now (make tiny, make test). You are working in python, so you could write tests in whatever way feels comfortable to you (nose? pytest?) and in follow-up I'd be happy to add a step to the CI.

joha2 commented 2 years ago

@paulfitz I integrated some tests. The file tests_runner.py in the analysis directory has to be integrated into the CI cycle. I think this is PR ready for merge. Please tell me whether there is anything missing. The tests can be improved later when needed. But I would postpone this to another PR. What do you think?

alanfwilliams commented 2 years ago

This works on my end. If you include the latest commits to master you should be able to add something like

- name: analysis test
  run: python analysis/tests_runner.py

to .github/workflows/main.yml and the CI should be integrated.

joha2 commented 2 years ago

@aw1231 thanks for testing! I could also add these lines into the yaml file in this branch. Would this help? Once the PR is merged, the CI could also perform the Python tests.

alanfwilliams commented 2 years ago

Yes, that would be great.

joha2 commented 2 years ago

@paulfitz The test fails with some syntax error in the Python file which I believe comes from installing Python 2.7 instead of Python 3.x. Is there a reason why CI is initialized with Python 2.7?

paulfitz commented 2 years ago

@joha2 fine to upgrade python to something contemporary.

joha2 commented 2 years ago

@paulfitz thanks for your fast response! I would like to use Python 3.8 since my system has this as system Python distribution and therefore I tested the code with it. I will update the yaml file accordingly.

joha2 commented 2 years ago

@aw1231 and @paulfitz finally :smile: Ready for merging. Thanks for your help!