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

Added todict() method to Command #67

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

Seemone
Copy link

@Seemone Seemone commented Aug 10, 2018

Adds a method to represent the sieve filters as dicts

usage example:

    jlist=[]
    if p.parse(filters):
        for r in p.result:
            jlist.append(r.todict())
        print(json.dumps(jlist),file=outfile)

Copy link
Owner

@tonioo tonioo left a comment

Choose a reason for hiding this comment

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

Please make your PR compatible with PEP8.


"""

j=OrderedDict()
Copy link
Owner

Choose a reason for hiding this comment

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

Could you respect PEP8 please?

j=OrderedDict()
comms=[]

if isinstance(self,TestCommand):
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

if self.children:
j.update({"children": []})
for child in self.children:
#target.write(("*" * (indentlevel+10))+'{"'+argname+'": "'+str(arg)+'"},\n')
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove this comment?

@codecov
Copy link

codecov bot commented Aug 17, 2018

Codecov Report

Merging #67 into master will decrease coverage by 2.12%.
The diff coverage is 15.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
- Coverage   84.07%   81.94%   -2.13%     
==========================================
  Files          10       10              
  Lines        1683     1734      +51     
==========================================
+ Hits         1415     1421       +6     
- Misses        268      313      +45
Impacted Files Coverage Δ
sievelib/parser.py 86.41% <100%> (ø) ⬆️
sievelib/commands.py 85.87% <13.46%> (-9.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d28ce4...284bd51. Read the comment docs.

@Seemone
Copy link
Author

Seemone commented Aug 17, 2018

Modified and checked against http://pep8online.com/
The only outstanding issue is a line too long in the original scope (out of scope of this PR) that it's too long because of an inline comment.

@tonioo
Copy link
Owner

tonioo commented Aug 17, 2018

Can you add some unit tests please?

BTW, I think you should be interested by this method: https://github.com/tonioo/sievelib/blob/master/sievelib/commands.py#L269.

@Seemone
Copy link
Author

Seemone commented Aug 17, 2018

I have no idea how to do it, sorry.

@Seemone
Copy link
Author

Seemone commented Aug 17, 2018

But it would be a good idea, since I already discovered a bug :)

@Seemone
Copy link
Author

Seemone commented Aug 21, 2018

I'm going to add an attribute name mapping layer that will change the keys of the arguments

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