Skip to content

Fix default namespace in PrettyPrinter.formatNodes #50

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

Merged
merged 2 commits into from
Apr 21, 2015

Conversation

chrisloy
Copy link
Contributor

This replicates the fix from #28 and applies it to the formatNodes method of PrettyPrinter as well. Existing behaviour is the same bug that @ashawley highlighted before - namely the addition of an erroneous empty xmlns="" at the top level of each node.

@ashawley
Copy link
Member

When I investigated #28 I noticed other instances of null default arguments for scope in PrettyPrinter that should have been TopScope. I only avoided fixing them to narrow the impact of my patch. Four months later, it seems there is very little risk fixing these. Fixing them seems to be the right thing to do.

@chrisloy
Copy link
Contributor Author

Thanks @ashawley. I think the only other one is here ... I'm happy to fold that change in too.

@ashawley
Copy link
Member

I saw that, too. I'll let @adriaanm determine if he wants a separate PR for that last null. Perhaps cleaning up the neighborhood is appreciated in this case.

@adriaanm
Copy link
Contributor

Happy to let you decide on this! This does seem like a nice occasion to apply the boy scout rule :-)

@chrisloy
Copy link
Contributor Author

I'll have a look this evening (UK time). If it is indeed as simple as it looks then I'll add that to this PR, if not let's do it separately. Thanks guys.

@chrisloy
Copy link
Contributor Author

Second commit added which also fixes (and tests) the behaviour when passing in a StringBuilder. I believe that's the last place!

@adriaanm
Copy link
Contributor

Excellent! Btw, our commit guidelines would recommend a title like "Default to TopScope, not null for name space binding." (The commit stats already tell you which file was updated, and "fix" is too generic.)

@chrisloy
Copy link
Contributor Author

Thanks @adriaanm, have tweaked the commit messages accordingly; I hope they read better now.

Assuming the build passes, do you reckon everything looks in order?

@adriaanm
Copy link
Contributor

Looks great to me -- thanks again!

@chrisloy
Copy link
Contributor Author

Hey @adriaanm - just wanted to check if you were waiting on any further action from me before merging this in? I couldn't find a specific contribution policy for this repo, but previous PRs looked to have been merged in with little formal process... Is there anything else you're expecting me to do?

@adriaanm
Copy link
Contributor

Sorry about the delay!

adriaanm added a commit that referenced this pull request Apr 21, 2015
Fix default namespace in PrettyPrinter.formatNodes
@adriaanm adriaanm merged commit 14888a2 into scala:master Apr 21, 2015
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.

3 participants