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

add option to check and warn or bail out on flowtext #53

Merged
merged 4 commits into from
Apr 18, 2016

Conversation

oberstet
Copy link
Member

@oberstet oberstet commented Apr 2, 2016

This fixes #49 in a pragmatic way

print("SVG input document uses {} flow text elements, which won't render on browsers!".format(cnt_flowText_el))
if options.error_on_flowtext:
sys.exit(1)

Copy link
Member

Choose a reason for hiding this comment

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

Right now it does not really matter (as it seems we don't touch flowRoots, even if they are empty) but in general I guess "compatibility checks" should be done somewhere at the end of scourString() as we most likely want to check Scours output for compatibility, not the input?

Copy link
Member Author

Choose a reason for hiding this comment

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

In general, I agree.

In this case, why do all the hard work processing/cleansing the SVG, when we bail out anyway.

@Ede123
Copy link
Member

Ede123 commented Apr 2, 2016

Why don't you include your tests in testscour.py? This will make it easier to keep track of functionality in future.

Also unittests should be minimal examples. Your SVG test files include a lot of unnecessary Inkscape cruft that has nothing to do with the actual test making it harder to find the cause of an issue if something breaks in future.

@oberstet
Copy link
Member Author

oberstet commented Apr 2, 2016

Also unittests should be minimal examples

No. The should be as close to real world as possible. And that (foten) means, the input file comes from Inkscape (saved as Inkscape SVG). Because designers just press save.

Why don't you include your tests in testscour.py?

That's a good point. I will fix that, though it'll require some refactoring .. scour doesn't use exceptions at all! And the unit test don't spawn the scour executable, but call directly into the Python module, so we can't test for exit code 1.

@Ede123
Copy link
Member

Ede123 commented Apr 2, 2016

No. The should be as close to real world as possible.

Well, I guess these are two different philosophies, both having advantages and disadvantages.
The most annoying problem with huge test files is that they often cause false positives when something totally unrelated changes (especially if the test conditions are not well defined) which is likely to cause unnecessary work.

That's a good point. I will fix that, though it'll require some refactoring .. scour doesn't use exceptions at all! And the unit test don't spawn the scour executable, but call directly into the Python module, so we can't test for exit code 1.

Yes, I see the need for a solution here, too, to check things like using file streams as input arguments. As you wrote we mainly test functionality in scourString(), but not the actual call of scour on the command line.

@oberstet
Copy link
Member Author

oberstet commented Apr 2, 2016

@Ede123 alright. unit tests added. I have only added minimal code .. no big refactoring. Too much work. Whitespace/PEP8 is also annoying and we should clean that up some time ..

@oberstet
Copy link
Member Author

oberstet commented Apr 2, 2016

FWIW, the whitespace commit in above only fixes trailing spaces .. the file contains tabs (probably even a mixture of spaces and tabs).

Many Python projects use this rule: indent == 4 spaces (no tabs). But PEP8 has more to say about whitespace ..

@oberstet oberstet merged commit 1a8ece2 into master Apr 18, 2016
@Ede123 Ede123 deleted the detect_flowtext branch February 25, 2017 18:47
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.

Option to warn about use of flow text elements
2 participants