LBT error in Python 3.5

benjaminrose commented 7 years ago

Looking at Travis CI, it looks like the ":" in the LBT acknowledgment is giving the Python 3.5 test issues. Since the web page is working, should we just drop Python 3.5 fromTravis?

onekiloparsec commented 7 years ago

Where did you see that the LBT acknowledgment was creating an issue? To me, travis job says it all:

The command "if [[ $TRAVIS_PYTHON_VERSION == 3.5 && $TRAVIS_BRANCH == "gh-pages" ]]; then git checkout gh-pages; git add database.json; git commit -m "Updated database.json"; git push -q https://astrobot:$GITHUB_API_KEY@github.com/astrofrog/acknowledgment-generator gh-pages >& /dev/null; fi" exited with 128.

Source

benjaminrose commented 7 years ago

I am new to Travis and not familiar with its error messages. I assumed it as and LBT error since that was the last "working correctly" line and it has an extra ":" that usually needs to be quoted in a .yaml file.

It appears that it is not an issue with LBT, so what do we need to fix so that Python 3.5 does not always fail?

onekiloparsec commented 7 years ago

Evil is inside the details. Just below the last line you mention it says in green The command "python parse_entries.py" exited with 0.. Hence the error is not in the parsing. As for the escaping of : it is not always necessary, as documented in YAML specs. But more easily you can see here that the LBT yaml is valid. Moreover, as a general rule, when a problem pops up, it's usually better to fix it rather than suppressing the test...

So what fails is this tricky command. I'll see if I can play with it in my fork.

onekiloparsec commented 7 years ago

I don't have the hands on the .travis-ci.yaml file to play with. But the file in it must be improved and simplified (what's the point of running it against 3 versions of python?!). @astrofrog should have a look.

benjaminrose commented 7 years ago

I am learning more about Travis CI and it looks like the issue is with updating the Astrobot updating database.json back to gh-pages, lines 22 through 27.

I feel like this line should not be running on pull requests. Maybe things are running as desired, but seeing failed tests on a PR just because it did not automatically ship seems like not the best solution.

onekiloparsec commented 7 years ago

The idea is good, but the process sucks anyway. It makes no sense to ask people to create YAML files to later on parse them to create a JSON database. Project like this allow to read YAML directly. However, you cannot AFAIK get a list of existing local files in javascript. Hence, you cannot retrieve the list of all YAML files in a directory dynamically. To me, when one needs to handle user input, one would be better of having a small (and free) real webserver with a nice webpage accepting new entries.