Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed linter warnings & moved linting to a separate CI job #677

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

LeXofLeviafan
Copy link
Collaborator

As per discussion in f00348e & #676

@jarun
Copy link
Owner

jarun commented Feb 14, 2023

Why can't we remove the else? What is this syntax try ... else? Why can't it be avoided?

@LeXofLeviafan
Copy link
Collaborator Author

Why can't we remove the else?

I don't follow… Removing it is exactly what I did 😅

What is this syntax try ... else?

else after anything other than if generally means "if the block executed normally"; i.e. else after a loop means "if loop exited on condition (not with a break)", and else after try means "if no error was raised".

…Yes, this isn't obvious at a glance, so this syntax is somewhat discouraged; it can be replicated explicitly by using boolean flags or return (which is what's going on here, so linter detected that using else here isn't necessary).

@jarun
Copy link
Owner

jarun commented Feb 14, 2023

Wouldn't the following work?

        except Exception as ex: 
            if not self.handle_view_exception(ex):
                msg = "Failed to create record."
                flash(
                    gettext("%(msg)s %(error)s", msg=msg, error=str(ex)),
                    "error",
                )
                LOG.exception(msg)
            return False

        self.after_model_change(form, model, True)
        return model

@LeXofLeviafan
Copy link
Collaborator Author

…You mean exactly like in this pull-request?
image

@jarun
Copy link
Owner

jarun commented Feb 14, 2023

OK. I saw the initial moved linting to a separate CI job changes.
Why do we need that?

@LeXofLeviafan
Copy link
Collaborator Author

Maybe [pylint] should be moved into a separate check (along with flake8); I doubt that it'd be much different between Python versions, and the result would be making clear whether it's linting or tests that doesn't pass.

That is, as of now it's hard to tell (without digging into logs) if the checks fail because of linting, or because the code doesn't work correctly.

@jarun
Copy link
Owner

jarun commented Feb 14, 2023

We do need to check the logs on a failure. Please make the code change only.

@LeXofLeviafan
Copy link
Collaborator Author

At this moment, you couldn't even tell that master is passing all tests because it doesn't pass linting; doing the linting separately would make it much easier to tell what needs to be fixed (and reduce the amount of attempts necessary to fix erroneous pull-requests).

@jarun jarun merged commit 7e02931 into jarun:master Feb 14, 2023
@jarun
Copy link
Owner

jarun commented Feb 14, 2023

OK. If that makes things simpler.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2023
@LeXofLeviafan LeXofLeviafan deleted the lint-fix branch August 24, 2024 17:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants