Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix #3094: Refactor JSLint into an extension #3143

Merged
merged 8 commits into from
Mar 28, 2013
Merged

Fix #3094: Refactor JSLint into an extension #3143

merged 8 commits into from
Mar 28, 2013

Conversation

TomMalbran
Copy link
Contributor

This request fixes the issue #3094 by refactoring JSLint into an extension. I also did several changes in JSLintUtils, now being the main file in the extension as a result of the refactor, to use a template for the table and use Language to identify JavaScript files instead of the extension. I was also able to move the JSLint submodule to the extension.

Note 1: I had to touch my git files to move the submodule, so not sure if that will work when merged.
Note 2: Not sure why it didn't renamed JSLintUtils to main and show the differences.

@ghost ghost assigned DennisKehrig Mar 15, 2013
@@ -0,0 +1,282 @@
/*
* Copyright (c) 2012 Adobe Systems Incorporated. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 2013, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is not really a new file, mostly a rename and changes, although GitHub disagrees for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... could be enough, I'm not sure, though.
@peterflynn When should a file have the current year? When it's a new path (there was no file with that name before) or when the content is mostly new?

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we only update the year if it's semantically a new file. Renames can keep the original year (even when git diff doesn't realize it's a rename... on the command line you can adjust the threshold for 'rename detection,' but I'm not sure if GH lets you get at those settings).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this pull request twice, since the first one I wasn't able to move the submodule, and on the first try Git mark this file as a rename, but not on this second try.

Copy link
Contributor

Choose a reason for hiding this comment

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

2012 seems fine, then. :) Thanks, @peterflynn!

@DennisKehrig
Copy link
Contributor

Awesome, I'll get to to this later this week!

@TomMalbran
Copy link
Contributor Author

The submodule movement didn't work as I expected and was giving me lots of problem over the last merge. It also didn't made much sense, since this is now an extensions and could be removed. Any other extension uses submodules to load the thirdparty code, so I decided to just remove the submodule and add the files to the branch. I hope that is ok.

I fixed the merge issues and fixed a few other things.

I also changed the path in the NOTICE. I haven't move it around so that the differences aren't much, but I could do if needed.

@DennisKehrig
Copy link
Contributor

Reviewing.

@DennisKehrig
Copy link
Contributor

JSLint isn't used by other extensions, it seems, so moving it is not a problem.

@TomMalbran: "Any other extension uses submodules to load the thirdparty code" - how do you mean that? To my knowledge, the only extension that uses Git submodules is "Continuous Compilation". Ironically (and luckily) for JSLint :)

@@ -44,7 +44,7 @@ respective licenses below.
Released under the MIT, BSD, and GPL Licenses.


- JSLint - ./src/thirdparty/jslint
- JSLint - ./src/extensions/default/JSLint/thirdparty/jslint
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch!

@DennisKehrig
Copy link
Contributor

@TomMalbran Great work! Works right away, and doesn't break any unit tests.

Please fix the minor issues mentioned above and merge with master again, then I'll run the unit tests again and merge it. Thank you so much!

@njx
Copy link
Contributor

njx commented Mar 22, 2013

Hey there--please hold off on merging this since today is the last day of the sprint and we're trying to close it down. We can merge it next week. Thanks.

@TomMalbran
Copy link
Contributor Author

What I meant before, is that every default extension that uses thirdparty modules, doesn't uses in a submodule way. The files are just included in the thirdparty folder.

I haven't checked any other extension before. I also didn't thought that it could be possible to add the submodule directly in the extension folder as the "Continuous compilation did". I will check into that, might be nicer to have the code as a submodule, but defined inside the extension.

@DennisKehrig
Copy link
Contributor

This only works for extensions with an own repository, though, so I think you did it exactly right. Especially since many other third party modules like jQuery are just copies, too.

@TomMalbran
Copy link
Contributor Author

Fixed the review comments and merged with master.

The main issue I was thinking of when using submodules in an extension is what would happen if the folder doesn't exists, when the extension isn't there. But since is a default extension and removing it would mean to remove it from the repository, it would be needed to remove the submodule too. So I think that re-adding JSLint as a submodule can be good, if you think is needed/better.

@DennisKehrig
Copy link
Contributor

Alright, merging!

DennisKehrig added a commit that referenced this pull request Mar 28, 2013
Fix #3094: Refactor JSLint into an extension
@DennisKehrig DennisKehrig merged commit f4af422 into adobe:master Mar 28, 2013
@TomMalbran TomMalbran deleted the tom/jslint-refactor branch March 28, 2013 21:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants