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

UMD code style #13801

Closed
wants to merge 3 commits into from
Closed

UMD code style #13801

wants to merge 3 commits into from

Conversation

hnrch02
Copy link
Collaborator

@hnrch02 hnrch02 commented Jun 12, 2014

This includes three rather controversial things:
1.

  1. Renames the argument of the UMD function to factory from o_o
  2. Subsequently re-enables requireCamelCaseOrUpperCaseIdentifiers for JSCS
    1. Re-enables the strict rule of JSHint
    2. Adds /* jshint strict: false */ to the function (as suggested by @XhmikosR)
  3. Replaces === in favor of == in the UMD function to comply with the currently used comparator with typeof in the rest of the codebase. (Ref)

This is why I have split this into two easy-to-revert commits, in case the hopefully vivid discussion following this PR concludes that something should be removed.

/cc @fat @cvrebert

(Remember, make love, not war <3)

hnrch02 added 3 commits June 12, 2014 06:00
- Rename variable of UMD function
- re-add requireCamelCaseOrUpperCaseIdentifiers to JSCS configuration file
- re-add strict: true to JSHint configuration file
@cvrebert
Copy link
Collaborator

/cc @XhmikosR

@cvrebert cvrebert added the js label Jun 12, 2014
@cvrebert cvrebert added this to the v3.2.1 milestone Jun 12, 2014
@cvrebert cvrebert changed the title Fix umd UMD code style Jun 12, 2014
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

This also includes the first commit from #13800, I'll remove it and rebase this when #13800 is merged.

@fat
Copy link
Member

fat commented Jun 12, 2014

My thoughts…

Renames the argument of the UMD function to factory from o_o

o_o is a more expressive variable then factory because it is a literal face. If eyes are the window to the soul… i feel that with this change you are effectively trying to crush the soul out of bootstrap.

The o_o pattern is represented across the top of every plugin we have. It's not a mystery what it does. It's just a dumb little code block to be compatible with different module systems. why not add a little character to it… maybe make someone who happens across it smile?

Re-enables the strict rule of JSHint

It is not a challenge to remember to add strict mode.

You would have to be completely black out to forget to add a use strict declaration… and now you're perverting the beginning of every file with this ugliness /* jshint strict: false */

Futhermore, there is no consequence to not adding strict mode.

Replaces === in favor of == in the UMD function to comply with the currently used comparator with typeof in the rest of the codebase

This is a good catch, thanks

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

  1. I like it when code has some soul and when moods manifest themselves in it, but trading this for consistency in a project of the scale of Bootstrap is not something I think is a reasonable approach. I'm not keen on factory really, we can use anything that doesn't fail our style checks, like oO or something.

  2. Again, this is meant to ensure consistency, also across a longer time span. We have seen that while refactoring, it can easily happen that small things like strict mode get axed.
    In addition, we shouldn't forget that there are people who not only use the distributed code of Bootstrap in the browser, but also the development environment and they might not know about strict mode and especially for these kind of people strict mode would be beneficial.

2.1) I agree, it is ugly to disable the JSHint option locally, but as I said, I think it's the lesser of the two evils.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

Futhermore, there is no consequence to not adding strict mode.

That's not true, see this extensive document about strict mode.

@fat
Copy link
Member

fat commented Jun 12, 2014

little consequence… i know what it does.

@fat
Copy link
Member

fat commented Jun 12, 2014

We have seen that while refactoring something, it can easily happen that small things like strict mode get axed.

huh? when?

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

I'm not talking about strict mode specifically, for instance just yesterday: #13791.

@XhmikosR
Copy link
Member

👍 from my side for all points. Consistency FTW :)

@fat
Copy link
Member

fat commented Jun 12, 2014

in a project of the scale of Bootstrap

tell me more about how to develop for a project the scale of bootstrap

o_o

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

We can't have a serious discussion if we don't have mutual respect.

@hnrch02 hnrch02 closed this Jun 12, 2014
@mdo
Copy link
Member

mdo commented Jun 12, 2014

@hnrch02 I don't think @fat meant anything mean by dropping the meme in there, so apologies if it came off as disrespectful. It's a small reminder that we're building things on the internet and while we should always take Bootstrap seriously, we are going to have fun doing this.

Me? I like using funny placeholder images, hipster ipsum text, funny commit messages, some snarky replies here and there, and shitty tweets to take my job here a bit less serious.

At risk of sounding like an arrogant asshole, Bootstrap has done wonderfully well these last ~three years. I see no reason to abandon some of our favorite aspects that making working on Bootstrap fun for @fat and myself. We love our contributors and owe them a lot, but no one has put more thought, time, or effort into this project than the two of us (though @cvrebert is catching up 😆) these last few years.

Please, continue to propose changes and new features, but recognize that we want the character of Bootstrap to be preserved. That to me is way more important than abiding by particular configurations and code guidelines.

<3

@mdo mdo removed this from the v3.2.1 milestone Jun 12, 2014
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

It's a small reminder that we're building things on the internet and while we should always take Bootstrap seriously, we are going to have fun doing this.

That meme wasn't about fun, it suggests very clearly that I am not in a position where I can talk about the way things are handled "in a project of the scale of Bootstrap". Even though this might be right, it is arrogant and IMO disrespectful. But no hard feels.

Me? I like using funny placeholder images, hipster ipsum text, funny commit messages, some snarky replies here and there, and shitty tweets to take my job here a bit less serious.

This is not what I was proposing to change. Even if stuff like this leaves the impression of me being a buzzkill, I'm actually not. I just like to be serious where it is appropriate and I feel very strong about certain things.

At risk of sounding like an arrogant asshole, Bootstrap has done wonderfully well these last ~three years. I see no reason to abandon some of our favorite aspects that making working on Bootstrap fun for @fat and myself. We love our contributors and owe them a lot, but no one has put more thought, time, or effort into this project than the two of us (though @cvrebert is catching up 😆) these last few years.

Again, I like the little things you do on Bootstrap to make working on it more fun, but this seems different. Removing rules from linting/code style checking for funsies just feels not worth it for me.

Please, continue to propose changes and new features, but recognize that we want the character of Bootstrap to be preserved. That to me is way more important than abiding by particular configurations and code guidelines.

I don't like being positioned as the guy who tried to kill the intelligent and beautiful Bootstrapia.
I was not trying to rip anybody's soul out. It's a goddamned UMD factory function that has been in Bootstrap for like three days 💢

@fat
Copy link
Member

fat commented Jun 12, 2014

@hnrch02 from what i've seen you are clearly a really smart guy, which is why it's frustrating to see you wasting time on trolly, overly pedantic issues instead of making actually meaningful contributions.

We are literally "discussing" changing the name of a private variable… and adding a comment to our code base. That's it.

it suggests very clearly that I am not in a position where I can talk about the way things are handled "in a project of the scale of Bootstrap"

That comment was not meant to be a personal attack on you. So my bad if it felt like that.

It was meant to be an attack on the discourse "we should do x because we are a big project."

I start looking for the exits when anyone says that; you, mark, chris, anyone.

Historically bootstrap has done a lot of things differently… and i believe this approach has helped to make bootstrap what it is today.

If you think about it like a company… when it's a startup it's really great and fun, and then people come in and start saying "we're a big company now, we should do x,y,z" – and before you know it you are slow and irrelevant.

My feeling on linting is it's a good tool to help you write better code. If it gets in the way of you writing better code however, it is stepping over the line.

In this case – it was stopping me from writing a variable I wanted to write. I wasn't writing the variable carelessly… It wasn't like I was snake casing something crazy "amd_factory_blah". That's an obvious, really hard thing to do in our codebase when literally every variable around you is camel case. I've never done it by accident. Ever. And catching it in a code review is super easy and painless to update.

As for the strict test, it's simply i didn't want to put a giant god damn comment in our source code to tell the linter that the method doesn't need to be strict. At the end of the day I consider myself a craftsman. I want to write the most beautiful code possible. And that comment is an eye sore.

There are other options: make that function strict or wrap the whole thing in another iife with strict mode. But the cleanest option to me at the time was to just remove a test… which wasn't really doing much.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

We are literally "discussing" changing the name of a private variable… and adding a comment to our code base. That's it.

That's also partly why I closed the PR. It's really not that big of a deal.

So I say no hard feelings and we forget about this really pedantic discussion and get back to maintaining this awesome project? 💌

If you think about it like a company… when it's a startup it's really great and fun, and then people come in and start saying "we're a big company now, we should do x,y,z" – and before you know it you are slow and irrelevant.

Very good point, BTW.

fat added a commit that referenced this pull request Jun 12, 2014
@fat
Copy link
Member

fat commented Jun 12, 2014

sounds good, added the strict mode back, wrapped everything in an iife and updated the ==

@mdo
Copy link
Member

mdo commented Jun 12, 2014

@hnrch02 Just a quick follow up, I didn't mean to paint you as the sole aggressor here. You do awesome work, are super approachable via comments here, and are pretty prolific across the project. Much appreciated. <3<3<3

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Jun 12, 2014

Aww, guise. I can feel the love 💖

@hnrch02 hnrch02 deleted the fix-umd branch June 12, 2014 18:48
cvrebert added a commit that referenced this pull request Jun 23, 2014
Will hopefully revert this reversion and land a fully-working version of UMD in v3.3.0.

Revert "some changes from #13801 - add strict mode back and =="
This reverts commit 2b302f6.

Revert "Fix regression of #10038 introduced by #13772"
This reverts commit e9d6756.

Revert "MD/CommonJS/Globals #12909"
This reverts commit 1c6fa90.

Revert "address #13811"
This reverts commit f347d7d.

Conflicts:
    js/carousel.js
    js/collapse.js
    js/dropdown.js
    js/modal.js
    js/tab.js
    js/tooltip.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants