X Tutup
The Wayback Machine - https://web.archive.org/web/20200916181931/https://github.com/nodegit/nodegit/pull/1023
Skip to content

/ nodegit

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

Use ESLint with a config base of eslint-airbnb-config-base #1023

Open
wants to merge 12 commits into
base: master
from
Open

Use ESLint with a config base of eslint-airbnb-config-base #1023

wants to merge 12 commits into from

Conversation

@cbargren
Copy link
Contributor

cbargren commented May 3, 2016

This replaces JSHint with the newer, more powerful ESLint. Some discussion has been had about this already at #1016 (comment).
@cbargren cbargren force-pushed the cbargren:eslint branch from 22f5764 to ebdfbed May 3, 2016
@cbargren
Copy link
Contributor Author

cbargren commented May 3, 2016

So I think it'll be a good idea to start babeling the test/utils/lifecycleScripts folders as well while I'm in here. This is inspired by the fact that db56d36 fixes the linter but is actually not fully supported as the 0oXXX octal syntax is new to ES6. Rather than use some weird parseInt magic, We can just go ahead with babeling the test folder so we don't have to worry about what syntax is supported yet. My thoughts for the new babel structure (with the help of @implausible) are as follows:

  • All un-transpiled javascript will move into a js folder at the root which will now contain lib, lifecycleScripts, test and utils. The generate folder could also be a candidate, though that will probably be a bit more complicated.
  • The js folder will be transpiled to the parent directory, putting the output folders exactly where they used to be. These folders will now be .gitignoreed and the new folders will be .npmignoreed.

The biggest problem I see here is that any outstanding js changes that will go in immediately after this are going to be a merge conflict nightmare. We'll have to work this in quickly after we agree on something to minimize immediate conflicts.

Thoughts?
cbargren added 10 commits Apr 29, 2016
@cbargren
Adding ESLint and related rc's and modifying scripts
9dfbedc
@cbargren
Fixing the install script so ESLint can run on it
4159f9a 
 ESLint reads returns outside of a function block as a parser error, killing the linter on that file. While returning the promise that's running is more explicit, it isn't necessary to hold Node open while it finishes running, as Node doesn't finish a task until all of its callbacks have completed.
@cbargren
Switching implied octal to explicit octal usage
f1751da 
`0XXXX` for octal is no longer valid and kills ESLint. Switching to `0oXXXX`fixes this
@cbargren
Addressing two small linter issues
df6a3fa
@cbargren
Switching to airbnb-base so we don't needlessly pull in the react plugin
78ac532
@cbargren
nuke .jshintrc
20e71f9
@cbargren
Fix linter errors and invalid code in tests/ dir
d8254a0
@cbargren
Moving keys into a directory
4976cf3 
This is so they can be more easily separated from babeled code in the test/ dir
@cbargren
Updating the .gitignore to match the new babel pattern
b63da4d
@cbargren
Babeling test dir and reorganizing js to make babeling easier
Loading status checks…
d53f7a5 
lib/ -> js/lib/
test/ -> js/test/
Of note is that test/keys/ is staying put as there is nothing to be babeled in there.
@cbargren cbargren force-pushed the cbargren:eslint branch from ebdfbed to d53f7a5 May 6, 2016
@cbargren cbargren changed the title [WIP] Use ESLint with a config base of eslint-airbnb-config-base Use ESLint with a config base of eslint-airbnb-config-base May 6, 2016
@johnhaley81
Copy link
Collaborator

johnhaley81 commented May 6, 2016

Why js/lib/ -> lib/ and not lib/ -> dist/ like we do now?
js/test/tests/diff.js Outdated
@@ -245,12 +245,12 @@ describe("Diff", function() {

it("can diff the contents of a file to a string with unicode characters",
function(done) {
this.timeout(0);

This comment has been minimized.

Sign in to view
@johnhaley81

johnhaley81 May 6, 2016

Collaborator

Is this needed?

This comment has been minimized.

Sign in to view
@cbargren

cbargren May 6, 2016

Author Contributor

Oops, I thought I took that out. I'll take that out.
@johnhaley81
Copy link
Collaborator

johnhaley81 commented May 6, 2016 •
edited

Oh, ok I'm following you guys now. That (js/lib) looks good to me.
@johnhaley81
Copy link
Collaborator

johnhaley81 commented May 6, 2016

@tbranyen what do you think?
@tbranyen
Copy link
Member

tbranyen commented May 6, 2016 •
edited

Changing the folder structure will be a rebase nightmare as previously outlined. I don't see a huge rush to bring this in immediately, but I'll try and review this weekend! Thanks for humoring me with this exercise, I know it's not the fastest thing to land, because it affects a lot.
@johnhaley81
Copy link
Collaborator

johnhaley81 commented May 6, 2016

This is definitely not time critical :)
@cbargren
Copy link
Contributor Author

cbargren commented May 6, 2016

Awesome! Yeah, the rebase nightmare for any pending PRs is definitely the biggest concern here. Thanks for checking it out!
cbargren added 2 commits May 6, 2016
@cbargren
Removing leftover debugger code
Loading status checks…
dfad1ef
@cbargren
Fixing test failure on AppVeyor
Loading status checks…
448cc14
@johnhaley81
Copy link
Collaborator

johnhaley81 commented Jan 27, 2017

Well, this is conflicting with like 100 files so @cbargren are you able to rebase this and clean it up?
@maxkorp
Copy link
Collaborator

maxkorp commented Jan 30, 2017

At this point, the actual file changes probably need to be reworked. FYI passing --fix to eslint helps with a LOOOOOT of the trivial stuff
@cbargren
Copy link
Contributor Author

cbargren commented Jan 31, 2017

I'll have to take a look at it again when I have some time. Yeah, a lot of these changes will probably have to be completely reworked, but it's really only worth doing if we're committed to getting it in immediately when it's ready, since the longer it waits, the more rebase nightmares it's going to create.
@gucong3000
Copy link

gucong3000 commented Jul 27, 2017

Should we just lint files that changed?
@gucong3000
Copy link

gucong3000 commented Jul 27, 2017 •
edited

https://www.npmjs.com/package/polyjuice

Converts .jshintrc and .jscsrc files into .eslintrc and vice-versa

polyjuice --eslint .eslintrc --to-jshint > .jshintrc
@implausible implausible force-pushed the nodegit:master branch from c345989 to c67b436 Aug 26, 2019
@implausible implausible force-pushed the nodegit:master branch from 2b7db46 to 69b010a Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@cbargren @johnhaley81 @tbranyen @maxkorp @gucong3000
You can’t perform that action at this time.
X Tutup