Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Src: Refactored modules to allow external coupling #522

Closed
wants to merge 24 commits into from

Conversation

leobalter
Copy link
Member

This is a really big commit.

I changed the way the modules are built to improve some process:

  • It makes easier to plug external modules like a new diff in the build process (See Improve diff implementation #364)
  • Modules won't rely too much on global-like variables
  • JSHint grunt process now checks for errors before the build (concat) process
  • Actually there's a double check with JSHint after QUnit being built

I detected some whitespace issues in the process but fixed only few, I'm planning to fix this later.

@leobalter
Copy link
Member Author

@scottgonzalez
Copy link
Contributor

This diff is nearly impossible to reason about. You should redo this in several small commits.

@leobalter
Copy link
Member Author

No problem, I'll split it and after reviewing you choose if it's needed to squash.

@scottgonzalez
Copy link
Contributor

Thanks. That'll make it much easier to review and provide feedback.

@leobalter
Copy link
Member Author

I'm still trying, but that's a really difficult split. :)

2014-02-03 Scott González notifications@github.com:

Thanks. That'll make it much easier to review and provide feedback.


Reply to this email directly or view it on GitHubhttps://github.com//pull/522#issuecomment-33970261
.

@leobalter
Copy link
Member Author

@scottgonzalez Did it. Now I have to say it was the most difficult split I ever did.

Some commits may look dumb, but I did the split step by step to make the PR easy to review.

Looking forward to a feedback. Thanks!

files: {
src: [ "test/**/*.js" ]
nonDist: {
gruntfile: [ "Gruntfile.js" ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think multiple levels like this works. It doesn't throw because "src" is a special property in this position, the others are ignored and considered custom properties. It is only linting the 7 files from concat.scoped.src and not the gruntfile or tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg, my fault. fixing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks

@leobalter
Copy link
Member Author

BTW, the w=1 option turns this patch way easier to diff.

https://github.com/jquery/qunit/pull/522/files?w=1

@jzaefferer
Copy link
Member

I suspect a lot of the whitespace changes aren't even necessary, since we have a rule of not indenting the first level inside a function scope. Under spacing in the style guide:

If the entire file is wrapped in a closure, the function body is not indented.

Apart from that I wonder if it wouldn't make more sense to go directly to AMD modules. jQuery Core is probably a better reference then jQuery Mobile, since we only need to distribute a single file in the end. The way jQuery Core includes Sizzle might also be a good model here for including external modules.

@leobalter
Copy link
Member Author

It's relative easy to remove the re-indentation from files.

I did this patch as a step to go to AMD and it's not the best solution. I'll try to improve this.

@leobalter
Copy link
Member Author

I reverted the indentation to follow the mentioned rule. I made it in a dumb commit and I can further apply it in the other commits.

At this point, I'm ok to close this PR and rethink in a better implementation.

@jzaefferer
Copy link
Member

Yeah, I think we need an approach that is in line with our other projects. Using AMD would make sense, but as you know, there's still a lot of discussions going on about AMD etc.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants