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

Semantically order attributes by default #60

Closed
Ede123 opened this issue Aug 14, 2016 · 7 comments
Closed

Semantically order attributes by default #60

Ede123 opened this issue Aug 14, 2016 · 7 comments

Comments

@Ede123
Copy link
Member

Ede123 commented Aug 14, 2016

Follow up to #59:

As already discussed there we should order attributes by default in output.
Furthermore a semantic ordering would be preferable to increase human readability.

The challenge at hand is to find a viable way to achieve a useful attribute order, even with future SVG specifications, namespaced extensions, etc. in mind.

@Ede123
Copy link
Member Author

Ede123 commented Aug 14, 2016

Right now I see two possible options:

  1. Only care about a minimal subset of the most common attributes. Order everything else after that in alphabetical order. This will undoubtedly leave some attributes ordered in a non-optimum way but should be easier to implement and is probably more future proof.
  2. Try to achieve a useful order for all attributes. This would already be pretty hard for the attributes specified in SVG 1.1 alone, however (if done right) it would be the better solution by far.

If we decide on the first option I'd suggest the following order:
id/class -> transform -> x,y,z,width,height,x1,x2,y1,y2,cx,cy,r,rx,ry,fx,fy ->d ->properties -> style -> everything else in alphabetic order

If we decide on the second option I'd follow the order in the SVG 1.1 specification (see for example the path element) with the exception to put id/class at the start. Like before everything else goes at the end in alphabetical order.

@dirk-thomas
Copy link
Contributor

I think both options are actually the same to implement. I would suggest a pseudo algorithm like this:

all_attr = {...}
known_attr = ['id', 'class', 'transform', 'x', 'y', ...]
ordered_attr = OrderDict()
for a in known_attr:
    if a in all_attr:
        ordered_attr[a] = all_attr[a]
for a in sorted(all_attr.keys()):
    if a in ordered_attr:
        continue
    ordered_attr[a] = all_attr[a]

I think the important part is that the attributes have some order. Your suggested attribute order sounds good to me.

@Ede123
Copy link
Member Author

Ede123 commented Aug 15, 2016

I think both options are actually the same to implement

With "easier to implement" I meant that the list of known_attr would probably be much easier to maintain for option 1. In terms of code complexity it's probably about the same...

@oberstet Any thoughts from you side?

@Ede123
Copy link
Member Author

Ede123 commented Aug 24, 2016

@dirk-thomas - Tobias seems to be busy lately but I think this is something we definitely want (Scour messing up the order of attributes on every run was something that bugged me a lot in the past).

Is your offer to provide a PR still valid? If yes, I'd be happy to review it. As you noted implementation for the two cases I suggested should be similar and since I have no clear preference, feel free to work on whatever seems more natural to you.

@Ede123
Copy link
Member Author

Ede123 commented Aug 26, 2016

I just noticed --order-attributes does not work on style attributes yet (and therefore don't have reproducible output yet).
This is something we should remember to add when working on this enhancement.

@dirk-thomas
Copy link
Contributor

Here we go: #105

@Ede123
Copy link
Member Author

Ede123 commented Aug 30, 2016

Fixed by #105. Thanks again Dirk!

Follow up (ordering of style attributes) is #106.

@Ede123 Ede123 closed this as completed Aug 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants