Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

*stickedOperators: remove documentation in preparation of deprecation. #366

Closed
wants to merge 1 commit into from

Conversation

mikesherov
Copy link
Contributor

No description provided.

@mikesherov mikesherov restored the removeleftRightDocs branch May 8, 2014 18:36
@mikesherov
Copy link
Contributor Author

@mdevils, @markelog now that I've added conditional expression spacing, there is literally no reason people should be using the *stickedOperators utilities. Here's the order of things that will happen.

  1. immediately remove the documentation (hence this PR).
  2. remove usage of the *stickedOperators in presets (PR coming soon).
  3. add an error (line 1, column 1) for each file being linted with these rules telling them to use the other rules right before releasing 1.5.0
  4. drop these rules completely right before releasing 2.0.0

@gustavohenke
Copy link
Member

Nice.
I always had problems with these rules; the new conditional expression spacing seems way better.

@gavacho
Copy link

gavacho commented May 9, 2014

How does one specify that var x = a + b; is valid but var x = a+b; is not without using the *stickedOperators?

@mikesherov
Copy link
Contributor Author

@gavacho, requireSpacesInBinaryExpressions.

@markelog
Copy link
Member

@mikesherov

for each file being linted with these rules telling them to use the other rules

We probably should add those rules in the error message, or even not remove the documentation just yet, but add deprecation notice with explanation what rules to use instead, since this question comes up a lot.

@Krinkle
Copy link
Contributor

Krinkle commented May 14, 2014

@gavacho @mikesherov I can't find anything about requireSpacesInBinaryExpressions. I assume you meant requireSpaceBeforeBinaryOperators and requireSpaceAfterBinaryOperators. So the recommendation is people remove disallowRightStickedOperators in favour of those. I actually had both (not sure why) and it was causing issues for things like return +str or return +!!foo.

@mikesherov
Copy link
Contributor Author

@Krinkle yes.

@Krinkle
Copy link
Contributor

Krinkle commented May 15, 2014

@mikesherov What about >, >=, <, and <=? It seems those don't work as expected when put in requireSpaceBeforeBinaryOperators and requireSpaceAfterBinaryOperators.

Filed #378.

@markelog
Copy link
Member

We really should add all those binary operators to the test suite

@mikesherov
Copy link
Contributor Author

@Krinkle yes, those are obviously bugs which should be filed as such.

@markelog
Copy link
Member

Strictly speaking they are not bugs, since support for them weren't promised :-)

@markelog markelog added the docs label May 25, 2014
@mikesherov
Copy link
Contributor Author

@markelog, you think at this point we're good to proceed with this?

@markelog
Copy link
Member

I think so –

We probably should add those rules in the error message, or even not remove the documentation just yet, but add deprecation notice with explanation what rules to use instead, since this question comes up a lot.

Thoughts?

@mdevils
Copy link
Member

mdevils commented May 29, 2014

I believe we should include deprecation notice in the output once for each deprecated rule if it exists in the user config.

@mikesherov
Copy link
Contributor Author

Ok, so it seems like:

  1. Output deprecation notice once for each deprecated rule pointing to the correct rules to replace them with.
  2. No longer perform the actual validations from those rules.
  3. Mark current rules as removed (rather than deprecated) in the docs, considering as of 1.5, their functionality is removed and they always throw errors.
  4. With the release of next non-patch version after 1.5, remove the rules completely from the codebase and docs.

@mikesherov
Copy link
Contributor Author

OK, I'm going to proceed as per my comment on Thursday.

@markelog
Copy link
Member

markelog commented Jun 2, 2014

@mikesherov sounds good to me

@mikesherov mikesherov added this to the 1.5 milestone Jun 5, 2014
@mikesherov
Copy link
Contributor Author

@markelog can you just take a quick look here and see if it makes sense. Please ignore the duplication. I didn't want to sink time into DRYing code that will be removed shortly.

* output deprecation notice with list of rules to use instead
* update documentation
@mdevils
Copy link
Member

mdevils commented Jun 11, 2014

I believe we should throw a warning in StringChecker once for the whole run.

@mikesherov
Copy link
Contributor Author

@mdevils we are then left with the same issue as #334

The problem here is that throwing an error makes sense for CLI tools like grunt and gulp, but for plugins, not as much. For example, the Sublime Text Plugin via SublimeLinter will silently fail if an error is thrown. Same goes for a lot of the other IDE's.

Unfortunately, while this should be the plugins responsibility to fix, it's just as easy for us have this be a normal linting error and be gauranteed to not break the world.

@markelog
Copy link
Member

It looks like it does not matter how we go on about it, result still will be unsatisfactory :-(. We need to figure out some way for 2.0 to make a nice output for these kind of messages, given that we probably will have a lot of them.

  1. We could make `configure` method of those rules to output these messages in the console, but that would be an ugly side-effect
  2. We could leave those sticked rules in, but beside they usual message we could output these deprecation messages
  3. We could throw, but output is ugly and plugins would silently break
  4. Or output deprecation message only for first lint file

Of these options, i like the second one (although i don't really like it), i know we should be noisy about this, but removing them is too radical for my taste.

@markelog markelog mentioned this pull request Jun 14, 2014
@mdevils
Copy link
Member

mdevils commented Jun 14, 2014

I'm agree with @mikesherov.

Let's not break compatibility with reporters and output an error for every file.

@markelog
Copy link
Member

I didn't purposed to break anything, for me, this scenario is preferable –

We could leave those sticked rules in, but beside they usual message we could output these deprecation messages

But in any case, it's good to move forward.

@mikesherov could you release the new version? It would requires you to create jscs-browser.js file, changelog, tag, etc; - see the scripts commands in package.json. If you do, please don't forget to remove warning in the readme doc before publishing.

@mikesherov
Copy link
Contributor Author

Ok. Will do. Tomorrow is Father's Day in the US, so I may not have time. If not, I'll release Monday morning. Thanks everyone for coming to a resolution on this!

@mikesherov mikesherov closed this Jun 16, 2014
@mikesherov mikesherov deleted the removeleftRightDocs branch June 16, 2014 12:07
@markelog markelog mentioned this pull request Jun 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants