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

2732#connection properties type-safe guarding #3009

Closed
wants to merge 14 commits into from

Conversation

MuraraAllan
Copy link
Contributor

@MuraraAllan MuraraAllan commented Dec 24, 2021

Related to #2732

Changes proposed in this pull request:

  • connection properties type-safe guarding

Checklist:

  • docs updated
  • tests updated
  • Changes updated

Allan Murara added 3 commits December 24, 2021 14:36
add type checking

add type checking qmail-queue

smtp_forward type checking

spf type checking

is_dead_sender if transaction does not exist return true

tarpit and do_lookups remove falsy check

get_rs hook_unrecognized_command remove falsy check
bump eslint ecmaVersion to 20

lint
@MuraraAllan
Copy link
Contributor Author

@msimerson LFR

I added some logs also to clarify we entered the "null guard at x function"

I just catch and fixed some other throw points in case of connection null.
I also saw some code here and there null guarding the same way for other variables, maybe a "remove unsafe null guard and add log" task would come handy, what do you think?

I believe I forgot something on CI 😨 Can you help me with that?

plugins/tarpit.js Outdated Show resolved Hide resolved
@msimerson
Copy link
Member

I don't think we need the additional logging. When these conditions are hit, it's not an error or unexpected, and log entries for ended transactions tend to be confusing.

Also, which is easier to read and looks better in the code?

falsy

    let results = connection.transaction?.results || connection.results;

w/o falsy

    let results = connection.results;
    if (connection.transaction != null) {
        results = connection.transaction.results;
    }

@lgtm-com
Copy link

lgtm-com bot commented Feb 2, 2022

This pull request introduces 2 alerts when merging 1a883ce into 4afc7b1 - view on LGTM.com

new alerts:

  • 2 for Useless conditional

@MuraraAllan
Copy link
Contributor Author

MuraraAllan commented Feb 2, 2022

@msimerson Sorry long time no see

Hey, I'm not sure if this error on CI windows 16x happens because of GCC, build-essential.

Fallback to falsy check
Removed logs
Type-guarded some other occurrences

@MuraraAllan MuraraAllan requested a review from msimerson February 2, 2022 16:46
@MuraraAllan MuraraAllan changed the title 2732#falsy check type 2732#connection properties type-safe guarding Feb 2, 2022
@MuraraAllan MuraraAllan closed this Feb 7, 2022
@MuraraAllan MuraraAllan reopened this Feb 7, 2022
Copy link
Contributor Author

@MuraraAllan MuraraAllan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like sqlite3 doesn't work on 16.x, I fallback ci-test-win to only 14.x, ok?

plugins/rcpt_to.host_list_base.js Outdated Show resolved Hide resolved
connection type checkingw

Signed-off-by: Allan Murara <allan.murara@gmail.com>
@msimerson msimerson mentioned this pull request Mar 30, 2022
3 tasks
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.

2 participants