Closed dmotylev closed 7 years ago
dmotylev
commented
7 years ago 
zaccone
commented
7 years ago Hi Dmitry,
Thanks a lot for all the work! I am going to merge it eventually, just travelling a lot at the moment. If you have another pull requests feel free to add them too!
dmotylev
commented
7 years ago I was going to investigate a couple issues your mentioned before (with include and exists). Will you reopen them as defects?
On Tue, 7 Feb 2017, 07:46 Marek Denis, notifications@github.com wrote:
Merged #20 https://github.com/zaccone/spf/pull/20.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/zaccone/spf/pull/20#event-951616279, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJj_9nUVW6waH9I4W_G5RZt-BBHDXeQks5raCFugaJpZM4LsTtF .
--
Kind regards Dmitry Motylev
zaccone
commented
7 years ago Some of the issues don't hold, as apparently later commits from the PR fixed them, but I still think there are two major issues right now:
https://github.com/zaccone/spf/blob/master/parser.go#L277-L278 .I reckon this should return true, Permerror, nil as per 5.2. "include", and the table at the bottom of the section.
https://github.com/zaccone/spf/blob/master/parser.go#L299-L302 - we should return true here, otherwise, as mentioned in the review, Parser.parse() will not match that (invalid, faulty) term and simply skip, whereas general rule of thumb is to return immediately (4.4. Record Lookup and 4.6. Record Evaluation).
I was going to fix it tomorrow, but I will be happy to accept PRs. Please, just let's try to make smaller PRs, otherwise it's hard to review and merge. Ideally, one PR per feature.
dmotylev
commented
7 years ago Tomorrow is great. From my point of view, API is pretty stable and it's safe to mention that fact in readme
On Tue, 7 Feb 2017, 08:49 Marek Denis, notifications@github.com wrote:
Some of the issues don't hold, as apparently later commits from the PR fixed them, but I still think there are two major issues right now:
-
https://github.com/zaccone/spf/blob/master/parser.go#L277-L278 .I reckon this should return true, Permerror, nil as per 5.2. "include", and the table at the bottom of the section.
https://github.com/zaccone/spf/blob/master/parser.go#L299-L302 - we should return true here, otherwise, as mentioned in the review, Parser.parse() will not match that (invalid, faulty) term and simply skip, whereas general rule of thumb is to return immediately (4.4. Record Lookup and 4.6. Record Evaluation).
I was going to fix it tomorrow, but I will be happy to accept PRs. Please, just let's try to make smaller PRs, otherwise it's hard to review and merge. Ideally, one PR per feature.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/zaccone/spf/pull/20#issuecomment-277936712, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJj__T_PX2LCUzdXmxL04NHJ8P7Gdqvks5raDAigaJpZM4LsTtF .
--
Kind regards Dmitry Motylev