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 feature to order XML attributes by key alphabetically while prioritizing xmlns attributes #685

Merged
merged 5 commits into from
Jul 7, 2023

Conversation

bweis
Copy link
Contributor

@bweis bweis commented Jun 28, 2023

I would love to discuss this feature. While technically this does agains the ethos of prettier, it is super common for xml files. Especially android xml layout files.

Open to a discussion and totally okay with closing this out if you disagree with the validity of its use case.

@kddnewton
Copy link
Member

I am open to this feature being a part of this plugin, but would need a couple of things to change.

First, please prefix the attribute name with xml. Prettier can get confusing otherwise.

Second, at the moment you're sorting based on the printed output. That interface is not meant to be relied upon. If we decide to change the structure of our doc AST, we shouldn't have to go change this. Instead you should sort the attributes before you print them and rely on the XML AST.

If you can make those changes, I think we can consider merging this.

Copy link
Member

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

Marking as "request changes".

@bweis
Copy link
Contributor Author

bweis commented Jul 6, 2023

I did attempt to do this at first actually. However it seems like the only interfaces for manipulating the "AST" were the from xml-tools/parser which gave us a CST. Proving much more difficult to manipulate given it's Concrete nature. Is there a built in util within the Prettier ecosystem for changing the AST before reaching the printer?

https://prettier.io/docs/en/plugins.html#developing-plugins

@kddnewton
Copy link
Member

@bweis I don't think you would change the AST before it gets to the printer, you would do it within the printer. Right now you're calling:

const attributes = path.map(print, "children", "attribute");

if (opts.sortAttributesByKey) {
  sortAttributesByKey(attributes);
}

parts.push(indent([line, join(separator, attributes)]));

but ideally you would be sorting before you print. You can do this in a lot of ways, but I would probably put the printed doc and the node itself into an object, as in:

const attributes = path.map((attributePath) => ({
  node: attributePath.getValue(),
  printed: print(attributePath),
}), "children", "attribute");

if (opts.sortAttributesByKey) {
  attributes.sort(({ node: left }, { node: right }) => {
    // do the sorting here, based on the AST nodes
  });
}

parts.push(indent([line, join(separator, attributes.map(({ printed }) => printed))]));

@bweis
Copy link
Contributor Author

bweis commented Jul 7, 2023

Ack. I misunderstood what you had meant. Hopefully this looks good.

@bweis bweis requested a review from kddnewton July 7, 2023 15:24
Copy link
Member

@kddnewton kddnewton 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, just needs a rebase

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