PR - Issue: 48851 - investigate and port TET matching rules filter tests(vfilter simple)

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50408

• Created at 2019-05-27 12:06:50 by aborah (@aborah-sudo)
• Merged at 2019-05-30 18:22:02

---

Investigate and port TET matching rules filter tests(vfilter simple)

Relates: Resolves: #1911

Author: aborah-sudo

Reviewed by: ???

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-28 04:45:23

rebased onto 722abc17a9a4a9a24fbe5181480e2579bf90ee86

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 13:15:09

Please, separate worlds properly while using https://en.wikipedia.org/wiki/Snake_case

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 13:16:31

Once again, you create not only users here

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 13:20:18

It is hard to read at that point.
It will be better if you'll use proper parameter names in the method (maybe, you can make it a function?)

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 13:23:27

Also, please, avoid these value[0], value[1] approach. It is not really descriptive.
You can use this approach instead:
for attr, value in [YOUR_LIST]:

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 14:19:02

1 new commit added

• Fixing Simon's comments

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 14:20:34

@droideck changes are done as per your suggestion

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 16:53:56

Could you please rename functions properly? They create users, right?

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 16:54:39

snake_case

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 17:02:05

snake_case

Looks like DS is not accept this value except facsimiletelephonenumber

ldap.OBJECT_CLASS_VIOLATION: {'desc': 'Object class violation', 'info': 'attribute "facsimile_telephonenumber" not allowed\n'}

but we can change is other places , as done

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 17:04:50

1 new commit added

• Fixing Simon's comments 2

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 17:05:21

@droideck changes are done as per your suggestion

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 17:19:24

It is not a fixture so it will be better if it'll have a verb. Also, please, fix other functions

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 17:19:45

telephonenumber should be divided too

[389-ds-bot]

Comment from vashirov (@vashirov) at 2019-05-29 17:26:34

snake_case

Attribute names are not allowed with the underscore. What are you suggesting exactly?

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 17:26:36

It is not a fixture so it will be better if it'll have a verb. Also, please, fix other functions

Can you please elaborate , i am confused what's wrong with the name here . I am not getting any error with pylint

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 17:46:35

snake_case

Attribute names are not allowed with the underscore. What are you suggesting exactly?

I was talking about variable names, of course. I see no reason to keep them the same as attribute names.
It will be more easy to read if they are in snake_case.
Words intelephonenumber are not divided. And realvalue too.

I understand it can look as nitpicking but I'd like to mention it once now to make the code more readable in the future. (initially, it was facsimiletelephonenumber which is tedious, IMO)

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 17:49:45

It is not a fixture so it will be better if it'll have a verb. Also, please, fix other functions

Can you please elaborate , i am confused what's wrong with the name here . I am not getting any error with pylint

Functions (especially in tests) makes more sense to name with a verb. Like create_users.
Like this, it is more clear about what the function does. If you name it just unknown_lang, it is pretty unclear.

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 17:55:02

1 new commit added

• Fixing Simon's comments 2

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-29 17:57:20

@droideck changes are done as per your suggestion

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 23:23:42

I think it makes sense to mention the negative point of the test case here, in the steps.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-29 23:23:52

The rest LGTM

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 04:18:30

1 new commit added

• Fixing Simon's comments 4

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 04:21:46

5 new commits added

• Issue: 48851 - investigate and port TET matching rules filter tests(vfilter simple)
• Fixing Simon's comments 2
• Fixing Simon's comments 2
• Fixing Simon's comments
• Issue: 48851 - investigate and port TET matching rules filter tests(vfilter simple)

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 08:19:49

rebased onto 121fbd117721f3c3048d10a2478d7a5a088ae498

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 08:23:42

rebased onto c82f238e74f6321e34cba6a07ee8db40aff45a9f

[389-ds-bot]

Comment from bsmejkal at 2019-05-30 11:37:32

I think it would be better to name the variable as 'new_schema' or 'my_schema' etc., because 'schama' can be easily mistyped and cause collision.

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 12:29:14

rebased onto 43060f11375f6b0f957f74a8117496c5aa225b9c

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 12:29:34

I think it would be better to name the variable as 'new_schema' or 'my_schema' etc., because 'schama' can be easily mistyped and cause collision.

changed

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-30 13:23:42

Maybe Pagure is broken but I don't see the change. I still see schama

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-30 13:25:20

Pylint says it has a trailing whitespace

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 13:33:18

rebased onto aa2649f

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-05-30 13:34:22

@droideck changes are done as per your suggestion

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-30 13:43:05

LGTM

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-05-30 18:22:02

Pull-Request has been merged by droideck

[389-ds-bot]

Patch
50408.patch