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

Add brace_style "none" option + tests for javascript, then cleanup all brace_style tests #554

Merged
merged 9 commits into from
Oct 6, 2014

Conversation

c32hedge
Copy link
Contributor

@c32hedge c32hedge commented Oct 5, 2014

Javascript side of fix for issue #538. The last 3 commits here are more of a cleanup + refactor,
so that there's a single block of tests for brace style instead of 4 big blocks to try and keep consistent.
I might be able to make the original change to the code and tests in python but don't trust my
rudimentary python skills to be the one to do the same refactor there if it's desired/needed.

- Move several tests not involving brace positions out of the brace position testing
  section, and eliminate duplication
- Ensure parallel tests exist for all 3 brace styles
- Add comments to clarify the sections are testing brace positions
- Move copy-pasted, commented out test up to opening brace position tests comment,
  and add note to update tests correctly if the case is ever supported.
Each removed test was an exact duplicate of the test 5 lines before it
looks very similar to what I plan to implement in my next commit
Abstracts out the opening and closing whitespace before opening braces
and before closing braces, respectively, so that the tests are all written in
one place instead of 4 huge blocks, 1 for each brace style.
@c32hedge
Copy link
Contributor Author

c32hedge commented Oct 5, 2014

Note that the refactor significantly increases the number of tests run even though they're all in one place now--several different permutations of whitespace are tested on the input for each one.

@bitwiseman
Copy link
Member

First, thanks! Great contribution. Solid idea to make the brace tests more consistent.

First glance review:

  • Change the + eo + structure into where you use @eo or some other easily recognized string and implement a modified bt() that replaces those strings with the appropriate values. For example btbr("a = function(){return a;};", "a = function()@eo{return a;@ec};", eo, ec). I don't mean this specifically, but you get the idea.
  • Implement python port (I know what you're saying about not knowing python, but we've done a lot of work to make the code line up and minimize the cost of maintaining parity.)
  • Implement the tests in python (You can do this. Seriously, it's just python. Take a swing at it.)

I might have some additional suggestions, but I'm really impressed with the minimalistic change to the product code, so probably not much else to do here.

@bitwiseman
Copy link
Member

Actually, if you haven't started the modification to the test code, skip that. #216 will address that and you've inspired me to make that the next thing I do. Just get the python implementation and the tests for it you can copy and paste from c32hedge@140b18e .

@c32hedge
Copy link
Contributor Author

c32hedge commented Oct 5, 2014

Alright. I did have some WIP at c32hedge/js-beautify@b7bde28 (wasn't quite there but addressed some gotchas you might run into)

@bitwiseman
Copy link
Member

Cool thanks.

@c32hedge
Copy link
Contributor Author

c32hedge commented Oct 5, 2014

(note that those 'gotchas' were specific to your original suggestion on changing the + eo + structure of the brace position tests, not necessarily #216 in general)

@c32hedge
Copy link
Contributor Author

c32hedge commented Oct 6, 2014

Done--implemented python side, and copied over tests from c32hedge/js-beautify@140b18e as requested.

@bitwiseman
Copy link
Member

Excellent! Thanks!

bitwiseman added a commit that referenced this pull request Oct 6, 2014
Add brace_style "none" option + tests for javascript, then cleanup all brace_style tests
@bitwiseman bitwiseman merged commit 54ca21b into beautifier:master Oct 6, 2014
c32hedge added a commit to c32hedge/js-beautify that referenced this pull request Oct 7, 2014
bitwiseman added a commit that referenced this pull request Oct 7, 2014
Update documentation of brace style options (related to #538 and #554)
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