The tests don't cover this, but I hacked up a little test script which runs both versions of the function against various test paths to make sure it worked as I intended.

There's one significant difference caused by the fix: the old code would be OK with a path that had stuff after the object ID, e.g.:

/project/issue/26/something

or:

/fork/bob/projects/issue/27/something

This code is not okay with that, it would break. However, I'm fairly sure other parts of Pagure don't allow for any path component after the object ID either. And since, according to lib.git.get_repo_namespace(), namespaces can contain /, there is really no way to both handle namespaces and allow extra trailing path components beyond hardcoding the possible 'object types', I don't think.

Hmmm. So since the routes all look like this:

@APP.route('/<namespace>/<repo>/')

I don't see how namespace can possibly contain a /. But in that case, what was the point of https://pagure.io/pagure/c/5e42ca6fdf9122f6de57c8105653834c91a036f8 ? What possible benefit does that have besides allowing / to be in the namespace name?

Oh, well.

The tests don't cover this, but I hacked up a little test script which runs both versions of the function against various test paths to make sure it worked as I intended.

That might be worth adding to the unit-tests, would you be ok with it?

according to lib.git.get_repo_namespace(), namespaces can contain /, there is really no way to both handle namespaces and allow extra trailing path components beyond hardcoding the possible 'object types', I don't think.

Since we dropped what we called pseudo-namespace, namespace shouldn't have any
'/' anymore, nor projects.
If we end up in that situation, we likely have a bug somewhere.

The tests don't cover this, but I hacked up a little test script which runs both versions of the function against various test paths to make sure it worked as I intended.

That might be worth adding to the unit-tests, would you be ok with it?

Well, that was what I planned to do, but the problem is that this is completely outside the main pagure app and it's not currently set up to be tested at all. You can't even import it cleanly from anywhere else. We'd have to give it its own mini unit test setup, right?

according to lib.git.get_repo_namespace(), namespaces can contain /, there is really no way to both handle namespaces and allow extra trailing path components beyond hardcoding the possible 'object types', I don't think.

Since we dropped what we called pseudo-namespace, namespace shouldn't have any
'/' anymore, nor projects.
If we end up in that situation, we likely have a bug somewhere.

OK. I can change it back to just assuming the first of any remaining path components is the namespace, I guess.

Hmmm. So since the routes all look like this:

@APP.route('/<namespace>/<repo>/')

I don't see how namespace can possibly contain a /. But in that case, what was the point of https://pagure.io/pagure/c/5e42ca6fdf9122f6de57c8105653834c91a036f8 ? What possible benefit does that have besides allowing / to be in the namespace name?

I'm sure I had something in mind when I wrote that, but I don't quite remember
why right now :D

OK. Well, I changed it back. Do you think this is OK now? Or do you have any issues with it?

Should we add a check here for the number of items remaining? and report if there is something odd?

It's looking good :)

Well, that was what I planned to do, but the problem is that this is completely outside the main pagure app and it's not currently set up to be tested at all. You can't even import it cleanly from anywhere else. We'd have to give it its own mini unit test setup, right?

That is true, we can do that in a separate PR though.

Eh, we could. If I've got things straight, there should only ever be 0 or 1 components left at the end.

But if we want to get into checking, I might rather prefer to do something like work from the list of expected object types somehow: we're already effectively hard coding these, because there's if obj == 'issue': (do stuff) ... if obj == 'pull-request': (do stuff) ... else: (raise exception), so really we can already only actually cope with specifically identified routes.

Just thinking out loud, maybe what would be nice is a dict where the keys are object types and the values are bound functions that get the actual object from the parsed values; then the path parsing logic just has to parse the path and find /(anydictkey)/ and it can then parse the path components before that point with a high degree of confidence and not care about any trailing ones, and we're not hard coding things any more than we already are anyway.

How's that sound? I could put it together tomorrow.

okay, well, here's my latest testament to over-thinking things...a very explicit and somewhat paranoid path parsing function, a bit of a refactor, and some tests. wdyt?

I'm really liking this, thank you very much!

Two small comments:

• Could you run pep8 on the code? And maybe give some spaces in the tests to make them easier to distinguish and read?
• Could you rebase on the top of master?

Thanks again :)

Now rebased, with tests moved to main tests/ directory, extended tests using the model fixtures, and doc / spec file / ansible play etc. updates for the rename.

pep8 complains only about module import order (which is unavoidable when using the sys.path edit trick that all other tests use) and line length. However, PEP-8 no longer truly requires a max line length of 80, and there are >80 character lines all over the place in Pagure. I usually go with a limit of 100; there's one single line that's over 100 here, and that's just to line up the pylint disable comments for ease of reading.

Note that assertRaisesRegexp was added to unittest in 2.7, but so was assertIn and the existing tests use that all over the place.

Also note I did not include the SQLAlchemy version requirement boilerplate that's in all the other tests because I think it's bullshit. Near as I can tell it's something @pingou wrote in 2012 for fedocal, and he's just copy/pasted it into pagure. But the requirement is already in requirements.txt that's used by setup.py to define the module's requirements, I really don't think there's any need to duplicate that in all the code files. And besides, it's in tests/__init__.py so there's really no need for every single test that does import tests (like this one) to do it as well.

Also note I did not include the SQLAlchemy version requirement boilerplate that's in all the other tests because I think it's bullshit.

It's required if you have multiple versions of sqlalchemy on the system, such as we had in rhel6. It tells python which of the two versions you want to use.
You are right though that this is less of a problem for pagure.

And besides, it's in tests/init.py so there's really no need for every single test that does import tests (like this one) to do it as well.

There is if you want to run the test manually using python tests/tests_....py which you can do :)

there are >80 character lines all over the place in Pagure.

Well I actually do try to limit those but I guess I miss some every once in a while and at the end they end up being quite a few. I would try to contest the "all over the place" though :)

None of these are really blockers for me (maybe just the 80 chars if you don't mind).
Would you mind rebasing one last time?

"There is if you want to run the test manually using python tests/tests_....py which you can do :)"

Um, but it's still doing import tests if you run it that way.

I'm not gonna do 80 character lines unless forced, I just don't think 80 makes any sense. No-one really considers PEP8 to enforce 80 any more, AFAICT. I find 100 a much more sensible number.