Skip to content
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

Misc improvements #47

Merged
merged 8 commits into from
Mar 22, 2020
Merged

Misc improvements #47

merged 8 commits into from
Mar 22, 2020

Conversation

kaste
Copy link
Collaborator

@kaste kaste commented Mar 21, 2020

I noticed I actually run a different version of this plugin since a long time now. Sorry, this is a rather unfocused PR.

Improvements/changes:

  • If there is a .mypy_cache folder in the project just use (respect) that.

  • Do not check a falsy value for cache-dir. Leave the automatic default to '', but if the user sets explicitly False, respect that. (Typical in the mypy.ini we can say where the cache is located, so it is good to somehow disable the automatic behavior.)

  • If there is no working dir (t.i. no open folder for that window typically) do not lint at all. (This fixes an edge case really, when working_dir is None we don't have a stable key for tmpdirs and hence the chance that unrelated stranded files share the same cache folder.)

  • Do not clean all cache folder on reload which is triggered on startup, and for example when SublimeLinter hot reloads. The caches are very expensive to compute, and so we try to keep them if possible (if the cache is used within the last 14 days). (That's a tricky first commit here, and I hope I didn't messed it up while squashing and preparing this PR.)

  • Capture the error-code and filename. code is supported via mypy's show-error-codes flag (maybe roughly a year old), filename is supported by SublimeLinter core also since a year or so. (That also means users can actually follow imports now. SublimeLinter accepts errors from multiple files in one go.)

  • Allow only one mypy process per cache (basically per project or working dir). This is a bug fix; SublimeLinter actually tries to run mypy on multiple files in the background, but mypy does not lock the caches. It is strictly forbidden to run mypy on the same folder concurrently, so we ensure that with a lock. (This is cheaply implemented but just enough since we're not "highly" concurrent here in this context, usually these processes would just overlap a bit.)

kaste added 7 commits March 21, 2020 19:58
The caches are *very* expensive to compute so we should not trash them on
every start of Sublime.

We of course trash everything if the plugin gets uninstalled, otherwise
we clean if the last access time (`st_atime`) is older than 14 days.

A couple of tricks here:

- The `except NameError` trick keeps the cache warm if SublimeLinter
  (or this plugin) reloads. Otherwise Python would clean up the
  directories.

- If you exit Sublime, it is always a fast exit. (Sublime wants to be
  fast!) And so the temporary directories are kept. However, on a cold
  load `tmpdirs` is obviously empty, we thus need to repopulate it.

  Unfortunately we cannot just pass strings around because Python would
  trash every `TemporaryDirectory` immediateley on garbage collection.
  So we still store these objects, and resurrected directories become
  `FakeTemporaryDirectory` just for the interface of having a `.name`.
Remove the trailing `:`, these are probably typos although SublimeLinter
is permissive here.

Remove `--incremental` which is outdated. Users can still provide this
option using `args`.
mypy actually operates with caches (probably even using sqlite) and these
aren't locked while running. This leads to various errors when we try to
run it concurrently.

SublimeLinter itself has no flag which forbids concurrent processing, so
we just lock it ourself. Creating and acquiring a lock using a
`defaultdict` is in itself not thread aware but it is simple enough here.
The user can set `cache-dir` to `False` to omit the arg from the command
completely. This is especially useful if the actual cache dir is configured
in the `mypy.ini` file.

Omit automatically in case the default location `.mypy_cache` exists.
Likely it is populated and the right thing.

In all cases do not manually edit `cmd`. Just set the setting and let
SublimeLinter do the rest.
@FichteFoll
Copy link
Collaborator

FichteFoll commented Mar 22, 2020

Thanks for these changes.

I didn't even realize --show-error-codes was added to mypy. Or maybe I did but quickly forgot about it.

Could you update the README for these changes as well?

Remove the trailing :, these are probably typos although SublimeLinter
is permissive here.

These were documented features for the defaults dictionary, but they might have been removed since. I don't quite remember what exactly they did, however.

@FichteFoll
Copy link
Collaborator

Also, you say that SL can interpret filenames in the output by itself, but do we want to change the default for --follow-imports?

@kaste
Copy link
Collaborator Author

kaste commented Mar 22, 2020

Ah, the README, will take a look.

The trailing : somehow magically determines the behavior if a setting can be or in fact is set multiple times, t.i. the user sets an array of things. But here these flags can only be set once I guess. (The exact rules I have to look up each time in the SublimeLinter tests actually. Iirc it is a simple system with bugs we keep for compatibility. I don't think I could explain it.)

We could of course change the default of --follow-imports. Technically this works fine, but mypy is so unbelievable slow, and I wouldn't use it unless the project is already or almost green because otherwise mypy will follow and report possibly hundreds of errors. Don't know really.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Mar 22, 2020

Mypy does have an incremental mode that runs in the background, but is harder to inferface with and I hope it hasn't been removed yet, but in the current world of LSP implementations and live analysis, I doubt that. This is tracked in #32.

@FichteFoll FichteFoll merged commit 51a43db into SublimeLinter:master Mar 22, 2020
@kaste kaste deleted the my-version branch March 22, 2020 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants