frontend: unify naming convention for safe methods

[nikromen]

Originally, safe methods were considered as methods raising ObjectNotFound for API (mainly with session.one() method). Now methods which returns None or ignores the error completely are also considered as "safe".

For the sake of consistency:

• *_or_none -> returns None if object was not found in the database
• *_safe -> methods that ignores error completely (they basically logs and/or are safe from exceptions)

and renaming some of the current safe methods to normal ones that aren't safe by the new rules

[nikromen]

the fail in CI (copr-common rawhide) seems unrelated to this PR (some problem with package python-six)

also these linter issues were there before 💀 bad linter! :D

[praiskup]

also these linter issues were there before 💀 bad linter! :D

Detecting that this is a function rename is a rather complicated task not implemented in csdiff. You need to be a bit more benevolent if you rename calls. :-)

[praiskup]

Can you document the style in CONTRIBUTE.md?

[praiskup]

Even though, ... I don't know. I don't feel anyhow "safe" when I'm calling "_safe" methods. And creating new doesn't make much sense to me. My personal feeling is that we should much better document the method behavior in docstrings....

[praiskup]

I also somewhat dislike that this "safe" concept leaks outside the frontend/ boundaries :-) :-)

[nikromen]

I also somewhat dislike that this "safe" concept leaks outside the frontend/ boundaries :-) :-)

hmm that's true... but what to do with the safe methods in them? keep them as they are or delete safe suffix?

Even though, ... I don't know. I don't feel anyhow "safe" when I'm calling "_safe" methods. And creating new doesn't make much sense to me.

As I understood it, they are safe because they catch not-found tracebacks and instead of reraising them (and causing traceback mess in the API calls -> would result to 500 I guess) it translates it to 404 with reasonable message so this is somewhat safe.

My personal feeling is that we should much better document the method behavior in docstrings....

I am not against getting rid of the suffixes completely and documenting it... I care only about clarifying the inconsistency :D we can shortly discuss it at standup

[FrostyX]

I am all for changing the status quo because nobody really knows what "safe" means (probably because on every place it means something different)

*_noop_safe -> methods that ignores error completely (they basically logs)
*_none_safe -> returns None if object was not found in the database
*_safe -> raises an exception (ObjectNotFound) if no object found

But this is IMHO too many cases

My favorite is _none_safe, I think it is very useful to have methods like these. Considering that @praiskup doesn't like the word "safe" (I get it) we could easily change the name to something else. For example SQLAlchemy has .one() and .one_or_none() so we could use this convention and have e.g. get_chroot_or_none(...).

*_safe -> raises an exception (ObjectNotFound) if no object found

I think we shouldn't have to care if we raised our custom exception or the default SQLAlchemy exception. If we do, we should IMHO fix our error handler instead. So, I would remove the suffix entirely.

*_noop_safe -> methods that ignores error completely (they basically logs)

I cannot make up my mind here but I guess having methods about which we know that they don't raise any exceptions, is a good idea. If we change the naming as I suggested above, we could say that "safe" means "safe from exceptions" and simply use _safe for this case.

However, in many cases _or_none is basically the same thing as "safe from exceptions", so I don't know if we want to distinguish them or not.

What do you think?

[praiskup]

What do you think?

I agree, and to your question - I think we should have _safe and _or_none.

Repeating my concern; we should stay on Frontend with this concept, and we should document it.

[praiskup]

_safe doesn't imply _or_none and vice versa, right? So once could think of a thing_or_none_safe?

[FrostyX]

I agree, and to your question - I think we should have _safe and _or_none.

+1

_safe doesn't imply _or_none and vice versa, right? So once could think of a thing_or_none_safe?

From what I've seen the _or_none is used in our code when we query and return things from the database and the _safe in sense of "safe from exceptions" is mainly used for doing things and not expecting results. So theoretically there could be thing_or_none_safe but I think we won't have any.

Repeating my concern; we should stay on Frontend with this concept

Looking at the PR changes, even the original code with _safe already leaked out frontend

and we should document it.

Absolutely.
I guess here https://github.com/fedora-copr/copr/blob/main/CONTRIBUTE.md ?

[praiskup]

I guess here https://github.com/fedora-copr/copr/blob/main/CONTRIBUTE.md ?

I'd vote for that one, yes.

[nikromen]

no progress

[github-advanced-security]

vcs-diff-lint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[nikromen]

I fixed also a buch of pylint errors with this since the linter was screaming at me with a lot of old errors that were already there so this PR is code-cleanup in general
@praiskup @FrostyX PTAL

[FrostyX]

LGTM but I would suggest running beaker tests against this just to be sure.

[praiskup]

LGTM but I would suggest running beaker tests against this just to be sure.

It's going to be done before the release this week, so merging.