Skip to content

Conversation

@oalbe
Copy link
Contributor

@oalbe oalbe commented Jan 25, 2016

As issue #282 stated, I moved the exercises to an exercises/ folder.

I decided to do this in a different branch in order to avoid this pull request being mixed in with the other one I made yesterday about doc files (since they were actually two but GH merged them into a single one).

If this is not a good practice please let me know, I'm not that experienced in contributing to projects.

@kytrinyx
Copy link
Member

This is a great practice! Thank you :)

I wonder if something changed with how Travis is setting up the environment that this is running on, since all the versions are failing. I'll open an issue and see if any of the python track maintainers can take a look at that before we merge any PRs.

@behrtam
Copy link
Contributor

behrtam commented Jan 25, 2016

Have you tested that ./test/check-exercises.pystill runs all the tests?

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

Actually I did, but it returned Ran 0 tests in 0.000s. OK and I have no idea why.

I mean, I am assuming it should have failed if it wasn't able to find the exercises paths.

@behrtam
Copy link
Contributor

behrtam commented Jan 26, 2016

The problem is that the test runner (test/check-exercises.py) assumes that the exercises are in the same directory glob.glob('./*/*_test.py'). Maybe it's a good idea to check the discovered test files against the listed exercises in config.json. This way we could also stop testing deprecated exercises.

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

@behrtam I found the problem and fixed it. Now the test are being ran and they all pass.

Now I was thinking of implementing the check against the config.json file before making the pull request. You chose: do I PR now and try to implement the rest later, or should I make a single PR?

@behrtam
Copy link
Contributor

behrtam commented Jan 26, 2016

I would do the check against the config in a separat PR.
You might need to rebase this branch, because some of the exercises where changed slightly (#292, #291, ...).

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

@behrtam I did the rebase and pushed the new changes to my fork (which updates this PR. Yes, I'm writing it so I get it fixed in my head). Could you please check that I didn't make a mess with all the rebase/commit/push -f/etc things?

@behrtam
Copy link
Contributor

behrtam commented Jan 26, 2016

I think you accidentally included two files (TEST.md and LEARNING.md) from your other PR #285.

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

Damn.

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

@behrtam I'm sorry, I tried for the last two hours to find a solution to the mess I made, but there does not seem to be any.

At this point, I think that deleting the PR and redoing everything from scratch is the only solution. Unless you know of something else.

@kytrinyx
Copy link
Member

If you rename your current branch to something else, e.g. git branch -m backup, then you can start over from master:

git checkout master
git checkout -b exercises-folder

Then cherrypick the commits you want, and then

git push -f origin exercises-folder

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

Starting over from master would mean auto-include the changes in the other pull request (which is, the two .md files). I did researches and it there not seem to be a way to erase those commit from a given branch. All I was able to find is a way to put the HEAD back to a given commit, but it's useless in this case.

And I never used git cherry-pick but it seems something that applies changes, not remove them. It's a mess.

@kytrinyx
Copy link
Member

No, if you start over from the most recent master on upstream then you don't get any of those pull requests:

git checkout master
git fetch upstream
git reset --hard upstream/master

@kytrinyx
Copy link
Member

You can also git rebase -i HEAD~n where n is the number of commits you want to go back, and then you can delete the commits you want to remove from the list.

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

On my fork the things now seem to look sane, but GH does not totally agree, apparently?

Edit: Oh, and I made sure that the test passed. Everything seem to work fine.

@behrtam
Copy link
Contributor

behrtam commented Jan 26, 2016

Sorry that I have to nitpick again, but you missed the single exercise test.

$ ./test/check-exercises.py word_count
Traceback (most recent call last):
  File "./test/check-exercises.py", line 78, in <module>
    main()
  File "./test/check-exercises.py", line 59, in main
    test_file = glob.glob('./{}/*_test.py'.format(exercise_path))[0]
IndexError: list index out of range

@oalbe
Copy link
Contributor Author

oalbe commented Jan 26, 2016

@behrtam Thanks for pointing that out. It works now, here's some output:

$ python check-exercises.py word-count
...........
----------------------------------------------------------------------
Ran 11 tests in 0.002s

OK

$ python check-exercises.py etl
....
----------------------------------------------------------------------
Ran 4 tests in 0.000s

OK

$ python check-exercises.py sieve
...
----------------------------------------------------------------------
Ran 3 tests in 0.000s

OK

$ python check-exercises.py luhn
..........
----------------------------------------------------------------------
Ran 10 tests in 0.000s

OK

@behrtam
Copy link
Contributor

behrtam commented Jan 26, 2016

There is one last really small (2x ".") change we have to do. If you have a closer look at the travis-ci build, it executes the tests from the root project directory ($ ./test/check-exercises.py), while you tested it from inside the test directory.
So the path should be changed from '../exercises/...' to './exercises/...'.
This again showed us that it would really be great to check against the config.json entries.

Thanks again for your persistence. We will get this PR done soon.

@oalbe
Copy link
Contributor Author

oalbe commented Jan 27, 2016

@behrtam I can do the change, problem is that I actually tried the single dot solution first, but I wasn't able to make the tests run. So I don't think I will be able to test it after the change (unless you don't have a suggestion here).

Should I do the (relatively) blind change anyway?

@behrtam
Copy link
Contributor

behrtam commented Jan 27, 2016

Do it anyway. The travis-ci build should show if it works if you can't test it locally.

@oalbe
Copy link
Contributor Author

oalbe commented Jan 27, 2016

Done. Let's see what happens...

@behrtam
Copy link
Contributor

behrtam commented Jan 27, 2016

travis-ci-job/105144407 All tests running like a charm. Thanks for your work!

behrtam added a commit that referenced this pull request Jan 27, 2016
Moved the exercises to the folder exercises
@behrtam behrtam merged commit a9bc2b5 into exercism:master Jan 27, 2016
@oalbe
Copy link
Contributor Author

oalbe commented Jan 27, 2016

Thanks to you and @kytrinyx for the patience. I'm sorry if I messed things up.

@kytrinyx
Copy link
Member

One of the wonderful things with git and github is that it's nearly impossible to mess things up. Sure, they'll get confusing, but that's just temporary :)

We're all good. I'm always happy to help people figure out how to solve things when they get themselves up git creek.

@oalbe
Copy link
Contributor Author

oalbe commented Jan 27, 2016

Thank you :)

@oalbe oalbe deleted the exercises-folder branch February 17, 2016 11:13
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.

3 participants