Can we make it search "up" for a config?
braver opened this issue · 35 comments
@kaste I work on a monorepo with a couple of typescript projects, so the project root is somewhere else depending on the file I'm currently working on. We use tslint type checking rules that require the project, otherwise I could run without. Is there a way to set it to look in folders "up" from the current file? I can (and probably should) fix this by setting up the repo more logically, or open the projects in ST in a more sensible way, but inquiring minds want to know.
Hm, you should unset config and project settings. tslint should then walk up searching for a config file.
K, thanks!
Of course Robben is out after not even 10 minutes 🙄
Reopen for visibility
Also, not setting the defaults is probably the better default.
Not setting those defaults is a good idea. Should work better for most people.
For me it still doesn't find the tsconfig though. Does it search up from the the file, or does it look in the ${folder}?
Please enable debug mode and log what cmd SL actually executes. Also note the cwd (working dir) bc that's usually important. 😀
Ping.
By default, TSLint looks for a configuration file named tslint.json in the directory of the file being linted and, if not found, searches ancestor directories. Check out the rules section for more details on what rules are available.
So it should work if we remove config and project defaults. Like you do in #33. Can you confirm?
@kaste I have been using my branch for a while now and it works. I do get an erred because it then doesn't do project linting and it can't use rules that need type information, so that's inelegant. I can probably set it up to do project linting by providing the arguments for my project, but it works well enough without. I would like to more elegantly deal with the erred situation though, because it's just warnings and not errors and the linter is fully functional.
Okay. Filtering stderr is relatively easy. As follows
def on_stderr(self, stderr):
stderr = re.sub(
'Warning: .+\n', '', stderr)
if stderr:
self.notify_failure()
logger.error(stderr)
So you basically remove whole lines from stderr which you think are okay. Then check (if stderr!) if stderr still has something on it.
You could do this conditionally as well
if self.get_view_settings().get('project', None):
return super().on_stderr(stderr)
Bc maybe if a user defines a valid 'project' setting, all warnings still indicate something meaningful.
Well that's annoying. The reported command in debug mode works on the command line but not in ST.
With
"args": [
"--project ${folder}/system-types/webclient/src/client"
]
I get error: unknown option --project <snip>/system-types/webclient/src/client'
That should probably be
"args": [
"--project", "${folder}/system-types/webclient/src/client"
]
or
"args": "--project ${folder}/system-types/webclient/src/client"
but not a combination of the types.
(Actually defining args via defaults is often way easier for the end-user. Just saying.)
Thanks, the args work now. I think I use about 10% of SL 🙄
By default, TSLint looks for a configuration file named tslint.json in the directory of the file being linted and, if not found, searches ancestor directories. Check out the rules section for more details on what rules are available.
So, this covers config detection, which works, it searches up to find the config. Which is why I'm getting the warnings. So, we shouldn't set config at all, removing that is 👍 . If someone has a setup that needs to configure this, there's the args.
- We shouldn't set the project by default because it doesn't accept anything to disable it.
- It also enables project-based linting which is excessively slow and heavy.
- The way we set it before doesn't even do anything? Might just be me again, not knowing how all this works. The arg works though.
Of course, it's sometimes nice for the end user if we expose the args via defaults. It's not nice for us though, because different versions of linters sometimes have different args and we get to figure that out. It's also not 100% nice for the end user, because it means some args can be set via defaults and others can not. It creates the wrong expectation, confuses people, and doesn't teach the user how to set other args that he/she might find useful. Also, setting args is the same for each and every linter, whereas the defaults are all over the place. Therefore, I'm very much against exposing args via defaults, unless there is a strong case for it.
So, #33 actually looks good if I suppress the warnings.
When using a type system I probably prefer the heavy thing. What I hate is, when it tells me my current file is green after a change, but I actually broken 10 other files bc I change a type. (That's my experience with VSCode TS, it tells you yeah, no problems, but actually you made a catastrophic change and 'broke' (of course not bc the typing is only cosmetics in TS) all other files.
Well, that's true. Still, I don't see how we can enable that by default here. You can enable it, and perhaps we should document, but we can't set that up for you because we cannot know where your project configuration is (it's not necessarily in ${folder}) and if we set it you can't disable it.
So, that's it? Can't have tslint --project in a monorepo?
Consider this:
myproject/
sub_project_A/
tsconfig.json
sub_project_B/
tsconfig.json
And myproject is the Sublime project, where ${folder} points to. There's no way to have files in sub_project_A get linted with sub_project_A/tsconfig.json and files in sub_project_B get linted with sub_project_B/tsconfig.json, and it's a wont-fix?
Edit
Though there's kind of another roadblock, where this plugin is unable to find tslint, if it isn't in myproject/node_modules/.bin (as in my case).
No you’re reading that wrong, you can do that. It’s just not the default.
How?
Linters take env and arg settings, it’s in the docs at sublimelinter.com.
I know, but my .sublime-project is in myproject. sub_project_A and sub_project_B are just directories inside, I can't configure tslint for each of them individually. Or can I?
Oh right, no, there is no way to determine dynamically and correctly determine what —project to pass. Your repo root doesn’t have to be your ST project root though. Also, if you want type feedback from the project, perhaps you’re better served by a build system.
Your repo root doesn’t have to be your ST project root though.
It doesn't, but the sub-projects are pretty tightly integrated, switching between ST windows to look at another sub-project would be quite unergonomic.
Also, if you want type feedback from the project, perhaps you’re better served by a build system.
I do have a type-checking and linting watcher running in a terminal, but this is the exact reason why there are editor integrations: don't have to save and wait (and sieve through the flood of errors/warnings from other files), and the editor highlights the problematic code nicely, instead of having to count rows.
Thankfully, this is the only monorepo I'm dealing with at the moment, and it's somewhat rare that I write something that my TSLint config doesn't approve, so I can live without this, but still.
One popular example where this may affect other SublimeLinter-tslint users is DefinitelyTyped. Each typing directory is a mini-project, with its own tslint.json, and if you're a regular contributor (and many are, since it's a huge repo), you'll likely have the root directory as your ST project.
Your repo root doesn’t have to be your ST project root though.
Now what does that mean? What works with 🤞 is, you have the project file in myproject. In the project settings you don't just add '.'to folders, but each subproject. Like
"folders":
[ // <- you see this is an array
{
"path": "./subA"
}
],
Now, the variable $folder within SL will point to the correct folder containing the file you're currently viewing. (This array is processed top-down, if you need to add the root folder as well, it must come last.)
So, you have one .sublime-project and one window. (This is the intention of the code, but probably nobody knows this, and test coverage is ...)
I do have a type-checking and linting watcher running in a terminal, but this is the exact reason why there are editor integrations
With build system I mean an ST integrated build system. You can probably even set one up to run on save.
Now what does that mean?
Your example here is about what I had in mind. That should work. Not super ergonomic.
It’s not that I’m unsympathetic, I work on a TS monorepo every day. But dynamically determining the project in such a situation, even running with the project flag at all, is a bit of a corner case. I love monorepos, but having the entire repo in your editor at all times isn’t ideal either. Project switching in ST is super smooth too. Or learn python and whip up a PR 😉
@kaste Thought about that, but ST docs say this about $folder:
The full path to the first folder open in the side bar.
Isn't "first" going to be a problem? Or does it mean something else? Too tired to try at the moment.
ST integrated build system
Okay, can be done, but it'll just print errors to an ST console, right? And still, on-save only. I already get basically the same level of reporting from my external watchers. But with colors 🙂
having the entire repo in your editor at all times isn’t ideal
Don't know, I enjoy having all the go-tos, and a shared workspace, where my open files are persisted together, and where I can have two panes side-by-side, one showing a file from A, and the other - from B, and having to open only one project on boot. It's the little things, but they add up.
Project switching in ST is super smooth too.
I'm using a window manager with workspaces, so it's easier to just switch between two of them, but that's simply a downgrade all round, compared to having the whole repo as one project. Well, okay, the sidebar tree maybe wouldn't be as deeply nested 😛
Or learn python and whip up a PR 😉
I wouldn't mind some DIY, but I just don't miss TSLint error highlighting that much, sorry 🙂 I've been working on this project for 3-4 weeks, and noticed only a few days ago that TSLint is missing. Went to investigate and got here, and decided to tell you about my situation. I'm guessing the consensus is that, indeed, this is "a bit of a corner case", not worth solving. Until it is, to somebody.
We ship an enhanced version of $folder.
Ah, thanks 🙂 Now I see that you have it in the docs. Went there initially, but didn't read past the blue note, went straight to ST docs.
However, I also have an app which is structured like so:
/foo/
tsconfig.json
tslint.json
bar/
tsconfig.json
tslint.json
With this .sublime-project:
{
"folders": [
{ "path": "/foo" },
{ "path": "/foo/bar" }
]
}What does ${folder} get expanded into when in the context of a /foo/bar/index.ts? Decided to try it, and I believe I'm getting /foo, instead of /foo/bar.
Okay, can be done, but it'll just print errors to an ST console, right?
No, it also adds markers inline at each error telling you what’s wrong. And you can jump between errors using the keyboard.
The first folder in folders that matches wins. So the root folder must be declared last.
@braver Had no idea that was a thing, assumed all these years that build systems was just a glorified per-project keyboard shortcut manager. Thanks for the tip!
@kaste Got it, thanks! It's not ideal that the child project is listed before the parent, but I can live with that.
On a related note, just found out that this linter doesn't do background checking at the moment. That explains me not noticing it that much. Looking forward to TSLint getting the stdin PR merged. Or babel-eslint getting TypeScript support, that would be even better.
Thanks again for your time!
Hm, the first folder in reversed(folders) is probably the better choice 🤔
Is it possible to select the deepest match? I think that would be the most intuitive behavior.
Deepest match has the problem that it is an automatic algo. The user would have no way to override such a "decision".
Hmm, maybe the choice of the algo itself could be configurable. But I suppose this is way too much effort for such an obscure feature. Whether to reverse or not - it would help my case a tiny bit, but may surprise/inconvenience others.