-
Notifications
You must be signed in to change notification settings - Fork 513
Unify and introduce polymorphic require of external plugins #1150
Conversation
what do you consider breaking here? Looks good to me otherwise! |
For example, you could define {
"additionalRules": "foo/bar"
} now, you can do it only like that: {
"additionalRules": "./foo/bar"
} otherwise jscs will consider this value as module name |
Is there any way to preserve the old behavior without introducing too much complexity? Would be a shame to have to hold off till 2.0 for this. |
@markelog Why we can't check module |
@mikesherov we could consider releasing 2.0 instead of 1.12, there is some tickets/pulls that could be done/landed, plus yeah, auto-fixing is not a breaking change, but its feels like a lot, it could justify the major in pre-semver times. Even today, in many minds version change perceived not by breakage but by the functionality enhancement. /cc @mdevils, @mrjoelkemp @zxqfox it still be breaking, basically, we can't distinguish between the path |
We've already said that autoformatting needs to be in the 1.x line. |
You have a firm stance on it, did you make any promises? |
No, no promises made, but we already discussed this. No reason to release 2.0 over such a minor bug. If you'd like to revisit that decision, we should discuss that on the ML, no? |
You right, will send a letter |
* Loads plugin data. | ||
* | ||
* @param {String|function(Configuration)} plugin | ||
* @protected | ||
@param {String|null|function(Configuration)} plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@markelog, why are you doing refactoring, adding documentation and so on here? I can't get the idea of this Pull-request. It's lost in tons of code changes. |
@markelog, |
Sorry Marat, just pulled the thread, i tried to make commits as atomic as possible though, you might be better of reviewing them individually.
That is what i have repeatedly said.
This PR mentions tickets that it would solve. |
Would be great if we split this to several PRs. I need a part of it to finish @markelog I know it's hard to split it. But still can we make a try? |
@zxqfox +1 |
How so? I don't follow. Is there something preventing you to review this by the individual commits? |
Only fear of incomplete code. |
Every commit should atomic, if there is any issues please let me know |
@@ -24,22 +24,130 @@ var BUILTIN_OPTIONS = { | |||
* @name Configuration | |||
*/ | |||
function Configuration() { | |||
/** | |||
* List of the registered (not used) presets. | |||
* @protected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newlines after descriptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zxqfox rule for that? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, there is an issue for that :( jscs-dev/jscs-jsdoc#85
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Nice work here @markelog! 👍 |
* @param {String|function(Configuration)} plugin | ||
* @protected | ||
@param {String|null|function(Configuration)} plugin | ||
* @private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why private
instead of protected
? You're overriding, so it is protected
.
6c0a7d7
to
ed0f85d
Compare
Fixes #1083, #1030, #109.
Also add more tests and simplification of configuration modules. Some of these changes might be considered as breaking.