Skip to content

Config colorize #42

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.

Sign up for GitHub

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

Jump to bottom
Merged
wbsoft merged 8 commits into from
Jul 11, 2015
Merged

Config colorize #42

wbsoft merged 8 commits into from
Jul 11, 2015

Conversation

@uliska
Copy link
Collaborator

@uliska uliska commented Jun 23, 2015

With HtmlWriter.use_id_or_class() the user now has better control
over how the <pre> element is exported. I did always have to
replace this manually, which was far from convenient.

I will add the respective options to Frescobaldi too.

uliska added 5 commits June 23, 2015 18:08
@uliska
colorize: Add use_id_or_class option
b0701e1 
This adds control to how the <pre> tag and the line numbers
are exported. I have always found it inappropriate that
Frescobaldi has <pre id="document"> hardcoded. python-ly allowed
to change the "document" part already, now also the id can be
changed for class.

I will add the interface to Frescobaldi too.
@uliska
colorize.py: Remove unneccessary whitespace
4be60e6 
This was done by my editor automatically, so I commit it
as it seems useful.
@uliska
Colorize.py: typo
d624bc1
@uliska
colorize: make id/class handling more consistent
7b8b6e9 
- Fix missing else clause (and rearrange logic).
- Don't hardcode fallback
- Make valid attributes more extensible
- change name of function and field name

Adresses comments
#42 (comment)
and
#42 (comment)
@uliska
colorize: Also make wrapping tag configurable
5ce9ded 
Addresses comment
#42 (comment)
@uliska
Copy link
Collaborator Author

uliska commented Jun 23, 2015

Yes, and with 7b8b6e9 this has become completely simple. Isn't this now ready to be merged?

@uliska
Copy link
Collaborator Author

uliska commented Jun 23, 2015

Ah, sorry for asking: How about the print and Python2/3 compatiblity?

@PeterBjuhr
Copy link
Collaborator

PeterBjuhr commented Jun 23, 2015

In Python 3 print is a function and a parentheses should be used:

print("print me")

@uliska
Copy link
Collaborator Author

uliska commented Jun 23, 2015

I know that, and I know that one may use workarounds to make code compatible with both 2 and 3. Unfortunately I didn't really follow what happened recently when you made Frescobaldi Python3 compatible, so I don't know if there's a convention to handle that.

Is maybe from __future__ import print_function the adaptor to handle print() in Python2?

Actually running a git grep print shows that using print is not common at all in Frescobaldi, so I suspect that this is not the appropriate approach at all?

@proski
Copy link

proski commented Jun 23, 2015

Please always use from __future__ import print_function when using print and Python 2/3 compatibility is desired. Otherwise, print() output may be different on Python 2 and 3. Frescobaldi is a GUI program, so it's no wonder that print() is uncommon.

@uliska
Copy link
Collaborator Author

uliska commented Jun 23, 2015

Thanks.
So what's the preferred way of notifying the user (of python-ly) of an undesirable situation (such as an invalid attribute name) that doesn't deserve raising an exception?

@PeterBjuhr
Copy link
Collaborator

PeterBjuhr commented Jun 23, 2015

In Frescobaldi print is preferably only used for debugging (you can't expect a user to be able to read the print output). In Python-ly I'm not sure if we have any real cases of print messages to a user. I've used some prints in the MusicXML subpackage but as far as I can recall they are all more or less for debugging.

@PeterBjuhr
Copy link
Collaborator

PeterBjuhr commented Jun 23, 2015

So what's the preferred way of notifying the user (of python-ly) [...]

I have been thinking myself that it could be desirable to notify the user about certain things regarding the MusicXML export.

I guess what we want is a system that works both for GUI and CLI users.

@uliska
Copy link
Collaborator Author

uliska commented Jun 23, 2015

Just realized that the CSS generation has to be updated according to the attribute (# for id and . for class)

@uliska uliska mentioned this pull request Jun 24, 2015
Merged
uliska added 3 commits June 24, 2015 02:47
@uliska
colorize: Fix print() usage.
fcb4117
@uliska
colorize: Properly end wrapping tag
d271bb3 
The closing part of the wrapper still was hard-coded to <pre>
@uliska
colorize: Produce appropriate wrapper CSS type
6feb894 
If the CSS is produced with a wrapper_attribute of 'id' the main CSS
will be an id type '#', if we have a 'class' it'll be a '.'
@uliska
Copy link
Collaborator Author

uliska commented Jun 29, 2015

@wbsoft I think this PR is now ready to be considered for merging.

@uliska uliska mentioned this pull request Jun 29, 2015
Open
@uliska
Copy link
Collaborator Author

uliska commented Jul 7, 2015

@wbsoft bump

wbsoft added a commit that referenced this pull request Jul 11, 2015
@wbsoft
Merge pull request #42 from wbsoft/config-colorize
f689f88 
Config colorize
@wbsoft wbsoft merged commit f689f88 into master Jul 11, 2015
@wbsoft
Copy link
Collaborator

wbsoft commented Jul 11, 2015

regarding the print(), I'd raise an exception if a developer provides a wrong value to a function. print() is normally not used in Frescobaldi, only for (temporary) debug purposes.

@wbsoft wbsoft deleted the config-colorize branch July 11, 2015 06:52
@uliska
Copy link
Collaborator Author

uliska commented Jul 11, 2015

Thanks for merging. I'll have a second look at the print-s

@uliska uliska mentioned this pull request Apr 5, 2016
Merged
bryantgeorge added a commit to bryantgeorge/python-ly that referenced this pull request Jan 6, 2020
@bryantgeorge
Merge branch '42-support-manual-measure-length-set' into 'stable'
16eaba9 
Resolve "Support manual measure length set"

Closes frescobaldi#42

See merge request bgeorge/python-ly!55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@uliska @PeterBjuhr @proski @wbsoft