Coder Social home page Coder Social logo

Comments (28)

sindresorhus avatar sindresorhus commented on May 22, 2024 1

@tnga We already support "xo": false in package.json to disable XO, so I guess we could maybe support "xo": true as an indication of XO being enabled. Do realize though, that if you use this on many packages, and then upgrade XO globally, you'll have to update all these projects, or they will be failing. That's the benefit of having dependencies being local. I'm curious why you don't want XO as a local dependency?

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

It's done this way so it doesn't activate on projects using something other than XO, like ESLint or JSHint, as that would conflict. I do see your use-case though. I'm open to suggestions on how we could solve this. Maybe have a command XO: Enable that would enable XO manually? If yes, should that manual enabling persist between sessions in that project? Or anyone have a better suggestion?

// @jamestalmage @lukeed @wmertens

from atom-linter-xo.

lukeed avatar lukeed commented on May 22, 2024

A simple, single command would make sense.

Another viable option would be a 'Lint on Save' (like your esformatter package).

A possible third option would be to expose a full settings panel with:

  • format on save
  • extensions to lint
  • disable if .jshintrc/.eslintrc found
  • file patterns to ignore
  • custom rules (global) -- reusable rules array across projects

from atom-linter-xo.

wmertens avatar wmertens commented on May 22, 2024

How about a default XO config that is used when none is defined in the
project, and a setting whether to only run XO when package.json has it?

Then you can specify your favorite things (screw semicolons, yey tabs,
obviously!), and all JS files you edit can get those settings, but by
default it doesn't impact you.

I suppose that setting a package.json in your home dir with an XO config
would have the same effect, so pretty much that, without actually having

the file.

Wout.
(typed on mobile, excuse terseness)

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

A possible third option would be to expose a full settings panel with:

Too much. I want to keep options to a minimum.

How about a default XO config that is used when none is defined in the
project, and a setting whether to only run XO when package.json has it?

👍 Sounds good to me.

@jamestalmage You good with this?

from atom-linter-xo.

jamestalmage avatar jamestalmage commented on May 22, 2024

How about a default XO config that is used when none is defined in the
project

XO core already has that, are we discussing something atom specific?

a setting whether to only run XO when package.json has it?

Agreed, but it should check to see if it's in package.json or locally installed (i.e. they installed without --save). If they did either, then try to run it.

from atom-linter-xo.

wmertens avatar wmertens commented on May 22, 2024

@jamestalmage but then you need to have some packages installed again,
where this issue is about using the one that ships with linter-xo

On Sun, Apr 3, 2016, 3:07 AM James Talmage [email protected] wrote:

How about a default XO config that is used when none is defined in the
project

XO core already has that, are we discussing something atom specific?

a setting whether to only run XO when package.json has it?

Agreed, but it should check to see if it's in package.json or locally
installed (i.e. they installed without --save). If they did either, then
try to run it.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#21 (comment)

Wout.
(typed on mobile, excuse terseness)

from atom-linter-xo.

jamestalmage avatar jamestalmage commented on May 22, 2024

a setting whether to only run XO when package.json has it?

....

but then you need to have some packages installed again, where this issue is about using the one that ships with linter-xo

If you are using the "only run when XO is in package.json" , I am suggesting having a local copy installed should also cause it to use XO

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

XO core already has that, are we discussing something atom specific?

It's about being able to just put in a default XO config (same as the one in package.json) in the Atom settings.

Agreed, but it should check to see if it's in package.json or locally installed (i.e. they installed without --save). If they did either, then try to run it.

👍

from atom-linter-xo.

jamestalmage avatar jamestalmage commented on May 22, 2024

It's about being able to just put in a default XO config (same as the one in package.json) in the Atom settings.

Ahh - I see. Yeah that's cool.

How does this plugin invoke XO, (via CLI or API?) - what do we need to do in XO core to make this happen.

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

How does this plugin invoke XO, (via CLI or API?) - what do we need to do in XO core to make this happen.

API. No changes in XO strictly required, but it would be nice to extract the logic of whether a project has XO from this plugin into XO core, so both the Atom and Sublime plugin could use it.

Sidenote: It seems this plugin already incorrectly only uses the bundled XO version instead of the local one when available. So we'll need to change it to do the same as the XO CLI.

from atom-linter-xo.

jamestalmage avatar jamestalmage commented on May 22, 2024

Sidenote: It seems this plugin already incorrectly only uses the bundled XO version instead of the local one when available. So we'll need to change it to do the same as the XO CLI

The issue of automatically switching for the local copy is a bit more problematic when using via API vs CLI.

The CLI switch works well because the primary consumers are humans, and humans are good at adapting on the fly. Where the CLI is used by build scripts, those are almost always packaged locally with the module anyways, and use the local CLI if they are npm run scripts. The fact that we ignore the global CLI is irrelevant to build scripts.

Doing automatic API switching is a bit more problematic. For example, if we add a method to the core API that extracts "the logic of whether a project has XO" from this module, then that method is only available in new versions of XO. If we then rely on that method from core, then this plugin breaks for old installations of XO.

I am not against the change. Just putting the possible complications out there.

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

@jamestalmage I didn't mean we do automatic switch in the XO API. That's indeed bad. I meant we should do the same as the CLI in this plugin, not the API. Maybe expose a convenience API function in XO for achieving it.

from atom-linter-xo.

jamestalmage avatar jamestalmage commented on May 22, 2024

Hmm. I guess I don't understand. You are discussing automatically loading up the locally installed XO version right? That means the API this plugin uses will change.

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

Oh, ok, I got it now. My mistake. Yeah, that would be a problem, but I think we can just enforce semver here and only support loading the same major version as this plugin bundles. So if this plugin bundles XO 1.0.0, it will only load xo@^1.0.0 locally in the user's project.

from atom-linter-xo.

jamestalmage avatar jamestalmage commented on May 22, 2024

Just an idea: what about creating an argv based interface. Instead of passing an options object, you pass an argv array, and get back an eslint report.

var xo = requireLocally('xo/argv-interface');

if (xo) {
  var report = xo(source, pkg, [
    '--esnext',
    '--global=foo'
  ]);
} else {
  xo = requireWithinPlugin('xo');
  x.lintFile(...); // We can use the API directly if we can't find local
}

It could share code with cli.js. I think our command line options are a lot more stable than the XO API is.

Just a thought.

from atom-linter-xo.

jamestalmage avatar jamestalmage commented on May 22, 2024

Or, possibly better. Let's just expose a new API method that we will support long term. You can't pass it any configuration options:

var xo = requireLocally('xo');

var report = xo.lintForIDEPlugin(filePath, fileSource);

lintForIDEPlugin would locate the appropriate package.json based on filePath, and it would always return the basic eslint report object (which is pretty stable).

I think we could commit to keeping that particular part of the API stable long term.

If we can't find the local plugin, then we are using the one packaged with the IDE plugin, and we can use the full API (because we know which version we have at that point).

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

👍 Good idea. Let's go with that.

from atom-linter-xo.

Pysis868 avatar Pysis868 commented on May 22, 2024

Sooo, tried reading this thread.

What should I do if I have a mixed project (RoR, no proper package.json file to have here) that I just want to have some (in)frequent XO linting done in Atom appear.

I have tried running the XO: Fix, Linter: Lint, Linter: Toggle, and Linter: Toggle Panel commands in Atom, and do not see the expected error status represented anywhere.

Background:
Right now I have what I believe to be the linting icon towards the left of the bottom status bar telling me I have "No Issues" in my currently opened JS file (it resides in an ignored directory, if that's important), I have xo installed globally on the CLI with brew on a Mac, and running it on the file from there tells me I have a syntax error in the same file.

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

@Pysis868 There's no support for that yet, hence why this issue is still open. It's not something I personally need, so it's waiting for someone to do a pull request adding it.

In the meantime, you could use the xo command-line tool to lint, as it works without a package.json.

from atom-linter-xo.

Pysis868 avatar Pysis868 commented on May 22, 2024

ok, thanks. Sorry, read #27's status and that tripped me up a bit about this issue's status.

from atom-linter-xo.

Pysis868 avatar Pysis868 commented on May 22, 2024

Actually, forgot to ask, I just added a sample package.json file with:

...
"devDependencies": {
  "xo": "*"
},
...

and no syntax errors (anymore!).

It still doesn't seem to be working this way either. Any tips? Is this possible to help my situation?

from atom-linter-xo.

Pysis868 avatar Pysis868 commented on May 22, 2024

Sorry about the posts, just want to show what stages are occurring here.

I just opened another JS file in Atom, and the XO linter panel opened with it! (Using the 'fake' package.json approach. Still don't know a better way than this one yet.)

So without creating a MCVE, I suspect the issue is that the file is excluded from source control, causing the plugin to not lint the file, and have opened issue #38 accordingly.
...
#38 is closed now. atom-linter setting fixed that!

from atom-linter-xo.

tnga avatar tnga commented on May 22, 2024

Hi ! for my point of view preview @jamestalmage 's proposal is a good idea and hope you have focused on it.

XO behavior that consist to just lint projects in which it have been explicitly defined to do the job is a really cool thing. So keeping in mine that behavior, a minimum step to enable XO to linting a project without have too include it as dev dependency can be to set a package.json like with just a XO's config mention.

// package.json
{
  xo: {}
}

This minimum requirement event for a (simple)"one file project" isn't too much comparing to use a command to enable it (which can't be very usable in case of moving/renaming 's project, or without an .xorc file or somthing like that. which do not respond to philosophy behind XO).
Noting that this can also help to identify folders that files can be linting by XO event without and explicitly mention of XO as dependency.

from atom-linter-xo.

marionebl avatar marionebl commented on May 22, 2024

I think the behaviour of "xo": false is a bit surprising already. For nested projects it means "inherit", for everything else "disabled". Adding new behaviour for "xo": true would make matters worse in my opinion - how should this behave for nested projects?

I'd propose deprecating "xo": false and replacing it with something like this. "root" means the directory the given package.json is placed in.

enabled=true

{
  "xo": {
     "enabled": true
  }
}
  • uses defaults or config found in root/package.json
  • does not search further up for config

enabled=false

{
  "xo": {
     "enabled": false
  }
}
  • disables linting for root/**/*
  • does not search further up for config

enabled=inherit

{
  "xo": {
    "enabled": "inherit"
  }
}
  • uses defaults or config found in first root/package.json where enabled !== 'inherit'

I am not sure on merging the inherit option onto the enabled key - perhaps those should be separate?

from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

I think the behaviour of "xo": false is a bit surprising already. For nested projects it means "inherit", for everything else "disabled".

No, "xo": false just means that package.json should not be considered the root for XO and it should continue looking upwards. You can think of it like this: By default, the package.json to use is auto-detected, setting it to false forces it to be skipped, and setting it to true forces it to be used. I think that's quite simple.

With your proposal, every project would have to have "xo": { "enabled": true }, which is sub-optimal.

from atom-linter-xo.

IssueHuntBot avatar IssueHuntBot commented on May 22, 2024

@issuehunt has funded $40.00 to this issue.


from atom-linter-xo.

sindresorhus avatar sindresorhus commented on May 22, 2024

Closing as Atom is abandoned.

from atom-linter-xo.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.