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

PR: Add cursor property #56

Merged
merged 10 commits into from
Apr 27, 2020
Merged

Conversation

goanpeca
Copy link
Contributor

@goanpeca goanpeca commented Jan 8, 2020

No description provided.

@goanpeca goanpeca changed the title PR: Add list property for updating cursor WIP: Add list property for updating cursor Jan 11, 2020
@goanpeca goanpeca changed the title WIP: Add list property for updating cursor PR: Add list property for updating cursor Jan 27, 2020
@goanpeca goanpeca changed the title PR: Add list property for updating cursor PR: Add cursor property Jan 27, 2020
self._url = url

def __repr__(self):
return 'url("%s")' % self._url
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@freakboy3742 what should be the canonical url repr?
I just used 'url("%s")' but it could be

"url('%s')" or 'url(%s)'

Copy link
Member

Choose a reason for hiding this comment

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

Given the context, I'd say that the quotes, while allowed by the standard, are redundant, so I'd leave them off the canonical form.

Copy link
Contributor Author

@goanpeca goanpeca Jan 27, 2020

Choose a reason for hiding this comment

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

True, but given:

Some characters appearing in an unquoted URI, such as parentheses, white space characters, single quotes (') and double quotes ("), must be escaped with a backslash so that the resulting URI value is a URI token: '(', ')'.

I felt was easier to simply quote and avoid having to add escapes. Thoughts @freakboy3742 ?

Copy link
Member

Choose a reason for hiding this comment

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

Ok - so I can see two alternatives here.

Firstly, we could consider ignoring ", ' and () characters entirely. Have you included these characters because it makes sense to include them, or because the standard actually requires them? Does the URL have to be URL encoded? Are escaped quote characters actually a risk?

If we can't ignore them, we should be formatting with %r instead of %s. That will include the external quotes, but will also autoescape the content of the string, including switching quotes between ' and " such that quoting will be minimized.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks pretty good; the url() parser could be simplified a bit, but the rest is fairly straightforward.

colosseum/declaration.py Outdated Show resolved Hide resolved
colosseum/parser.py Outdated Show resolved Hide resolved
@goanpeca
Copy link
Contributor Author

Looks pretty good; the url() parser could be simplified a bit, but the rest is fairly straightforward.

Updated, but still have a question

colosseum/parser.py Outdated Show resolved Hide resolved
self._url = url

def __repr__(self):
return 'url("%s")' % self._url
Copy link
Member

Choose a reason for hiding this comment

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

Ok - so I can see two alternatives here.

Firstly, we could consider ignoring ", ' and () characters entirely. Have you included these characters because it makes sense to include them, or because the standard actually requires them? Does the URL have to be URL encoded? Are escaped quote characters actually a risk?

If we can't ignore them, we should be formatting with %r instead of %s. That will include the external quotes, but will also autoescape the content of the string, including switching quotes between ' and " such that quoting will be minimized.

colosseum/declaration.py Outdated Show resolved Hide resolved
@goanpeca
Copy link
Contributor Author

Firstly, we could consider ignoring ", ' and () characters entirely. Have you included these characters because it makes sense to include them, or because the standard actually requires them?

The standard says:

Quotes are required if the URL includes parentheses, whitespace, or quotes, unless these characters are escaped, or if the address includes control characters above 0x7e .
Does the URL have to be URL encoded? Are escaped quote characters actually a risk?

So yes I guess they need to be included

If we can't ignore them, we should be formatting with %r instead of %s. That will include the external quotes, but will also autoescape the content of the string, including switching quotes between ' and " such that quoting will be minimized.

I will try this then

@goanpeca
Copy link
Contributor Author

Yes - although I'm still a little confused on the condition where getattr(self, name) will succeed but getattr(self, '_%s' % name) will fail...

Ok, will add another try then 🤷‍♂

@goanpeca
Copy link
Contributor Author

So yeah, I think we can ignore the "special characters" for now @freakboy3742

@goanpeca
Copy link
Contributor Author

This one is ready for review @freakboy3742

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This all looks good; however, I have to wonder if we're actually getting anything out of the ImmutableList structure. Ok, sure, it's for defining properties that shouldn't be modified... but ultimately... does it matter if they're modified? What API interaction are we actively circumventing by putting that protection in place?

@goanpeca
Copy link
Contributor Author

goanpeca commented Mar 1, 2020

I guess the idea is that when an user requests a property by providing an immutable object we convey that modifying directly wont have an impact on the overall css. By giving the user a muta le object we might signal that modyfying that object has a baring on the style. That was my rationale for it.

@freakboy3742
Copy link
Member

Ok, sure - but would it? I mean, list-based properties are things like fonts and cursors where you are defining a fallback list (try this fancy font, then try this less fancy font, then try this simple font we can guarantee will exist).

I can only think of two ways that this could be potentially confusing:

  1. They might expect the style change to take effect immediately.
  2. There wouldn't be any validation on the value they add

The first isn't a huge concern to my mind - styles are applied when they're applied; as long as there's a clear "APPLY STYLE" entry point, then we train people that the style is just a data representation.

The second is a little more significant, but I'm not 100% convinced it's worth the complexity of the implementation.

However, some of that might be because the specific implementation here is a bit more complex than it needs to be. You've implemented ImmutableList as a wrapper around a tuple; however, you could also do it by subclassing list, and overwriting __setitem__, append, insert etc to raise an exception. You then get all the default behavior of list, but without any ability to modify the list contents.

@goanpeca
Copy link
Contributor Author

goanpeca commented Mar 3, 2020

You've implemented ImmutableList as a wrapper around a tuple; however, you could also do it by subclassing list, and overwriting setitem, append, insert etc to raise an exception.

That sounds equally "complex", unless we are talking performance-wise

But sure, I can do that too 🤷‍♂

@freakboy3742
Copy link
Member

You've implemented ImmutableList as a wrapper around a tuple; however, you could also do it by subclassing list, and overwriting setitem, append, insert etc to raise an exception.

That sounds equally "complex", unless we are talking performance-wise

The key difference is that if you subclass list, you only need to override and break the methods that you want to prohibit; the implementation of each subclassed method (and there aren't that many) is raise Exception("you can't do that"). You get a fully working implementation of __getattr__, __repr__, index, and everything else that is read-only for free. If you use a proxy, you have to reimplement (or at least proxy) all the basic sequence functionality.

There may also be a small performance boost, but honestly I'd be surprised if that actually survived the process of subclassing :-)

@goanpeca
Copy link
Contributor Author

goanpeca commented Apr 27, 2020

Should I disable also the dunder methods @freakboy3742 ?

'__add__', '__delattr__', '__delitem__', '__delslice__', '__iadd__', '__imul__',  '__mul__',  '__new__', 

'__reduce__', '__reduce_ex__',  '__reversed__', '__rmul__', '__setattr__', '__setitem__', '__setslice__',  

'append', 'count', 'extend', 'index', 'insert', 'pop', 'remove', 'reverse', 'sort'

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good - thanks!

@freakboy3742 freakboy3742 merged commit 8226112 into beeware:master Apr 27, 2020
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