Side navigation
#10753 closed enhancement (fixed)
Opened November 11, 2011 03:40AM UTC
Closed December 09, 2011 02:31AM UTC
Last modified March 10, 2012 12:36AM UTC
inline the evalScript function in manipulation.js as it's only used once
Reported by: | mikesherov | Owned by: | mikesherov |
---|---|---|---|
Priority: | low | Milestone: | 1.7.2 |
Component: | manipulation | Version: | 1.7 |
Keywords: | Cc: | ||
Blocked by: | Blocking: |
Description
As far as I can tell, there is no reason to have this be a named function and can be inlines. Can save some bytes, so... 1.8?
Attachments (0)
Change History (15)
Changed November 11, 2011 04:06AM UTC by comment:1
Changed November 11, 2011 12:11PM UTC by comment:2
Please, try and not be overzealous. It makes sense to have this portion of the code in a separate function from a maintenance perspective. I like the idea of saving bytes like anyone else but, by the same reasoning, jQuery itself would be one big function ;)
As a side note, wouldn't it be more productive to have a code review in an oksoclap or google docs rather than a myriad of tickets and pull requests? That way, you can pinpoint the opportunities for DRY and byte savings and yet have the whole team review it first before making a PR for each line :)
Changed November 11, 2011 01:12PM UTC by comment:3
jaubourg, from my perspective, when a function gets called twice, that's when it makes sense to have it be a separate function.
If you feel otherwise, as maintainability is mostly subjective, I'll respect that and defer to your expertise, but the idea that inlining a function that only gets called once would lead to jQuery being one big function is a slippery slope argument. On the flip side, I can make a slippery slope argument that all callbacks should be defined in there own functions in case we ever need to reuse them.
With regards to using google docs, I'm cool with that, I wasn't aware that was something commonly done by the team. How do I go about getting the team's attention if I create a google doc for all those changes?
Changed November 11, 2011 01:13PM UTC by comment:4
P.S. thanks for the feedback. I appreciate the time you're taking to help get me to be my most effective.
Changed November 11, 2011 01:14PM UTC by comment:5
As far as timing goes, I'd like to wait until we finish the crush of 1.7.1 to keep the pull queue clean. These could wait for 1.8 unless they're critical for other reasons such as fixing bugs.
Changed November 11, 2011 01:16PM UTC by comment:6
No problem. I'll close this PR till then.
Changed November 11, 2011 03:14PM UTC by comment:7
I dunno who's in charge of this but maybe getting onto the jquery bug team mailing list would be a good first step given how active you've been lately :)
Dave?
Replying to [comment:3 mikesherov]:
jaubourg, from my perspective, when a function gets called twice, that's when it makes sense to have it be a separate function. If you feel otherwise, as maintainability is mostly subjective, I'll respect that and defer to your expertise, but the idea that inlining a function that only gets called once would lead to jQuery being one big function is a slippery slope argument. On the flip side, I can make a slippery slope argument that all callbacks should be defined in there own functions in case we ever need to reuse them. With regards to using google docs, I'm cool with that, I wasn't aware that was something commonly done by the team. How do I go about getting the team's attention if I create a google doc for all those changes?
Changed November 11, 2011 04:15PM UTC by comment:8
@jaubourg the bugs team mailing list was recently made public for reading, but only bugs team members can actually post to it.
@mikesherov - I've sent you an invite to the bugs team mailing list. This will give you posting permission
Changed November 11, 2011 09:28PM UTC by comment:9
@rick I knew about the list being made public, I was obviously talking about posting permission
Changed November 12, 2011 05:59PM UTC by comment:10
component: | unfiled → manipulation |
---|---|
milestone: | None → 1.8 |
owner: | → mikesherov |
priority: | undecided → low |
status: | new → assigned |
Changed December 06, 2011 01:29PM UTC by comment:11
resolution: | → wontfix |
---|---|
status: | assigned → closed |
this will be fixed when JSHint is implemented here: https://github.com/jquery/jquery/pull/602/files#L14L359
Changed December 09, 2011 01:32AM UTC by comment:12
resolution: | wontfix |
---|---|
status: | closed → reopened |
Changed December 09, 2011 01:32AM UTC by comment:13
status: | reopened → assigned |
---|
Changed December 09, 2011 02:31AM UTC by comment:14
resolution: | → fixed |
---|---|
status: | assigned → closed |
Fix #10753. Inline evalScript as it's only used in one place
Changeset: 07866a04ddcac0b912cd0a7e382fea8667b32d0e
Changed December 10, 2011 06:37PM UTC by comment:15
milestone: | 1.8 → 1.7.2 |
---|
https://github.com/jquery/jquery/pull/591