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

Fix syntax file extensions and custom color scheme #41

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

parasyte
Copy link
Contributor

This fixes compatibility with the FileIcons package and handling of the color scheme.

- Fixes running the package in developer mode from the `Packages`
  directory in SublimeText 4.
- Ignores the Python cache directory that is created when running the
  `other` scripts from the terminal.
Changes the syntax scope to `text.csv.*`, e.g. `text.csv.rbcstn44`
instead of the unusual `text.rbcstn44`. (What is a "rbcstn44" file,
anyway?)

Also sets the file extensions, allowing the FileIcons package to work.

Closes mechatroner#34
subrepo:
  subdir:   "json5"
  merged:   "43901a8"
upstream:
  origin:   "https://github.com/dpranke/pyjson5.git"
  branch:   "main"
  commit:   "43901a8"
git-subrepo:
  version:  "0.4.9"
  origin:   "???"
  commit:   "???"
- The color scheme path could not be found due to a syntax error
  (case sensitivity).
- When pregenerated grammars are used, the wrong settings path was
  chosen.
  - And auto detection/reloading package settings wasn't working when
    Rainbow CSV syntax was auto-detected by file extension, because then
    it was no longer "plain text".
- Use json5 to parse Syntax-specific settings and set the color scheme.
  - Fixes a bug where modifying the Syntax-specific settings would
    conflict with how the color scheme was enabled and disabled:
  - A pre-existing settings file would prevent the color scheme from
    being used.
  - Disabling the custom color scheme would remove all settings,
    including user preferences like disabling `word_wrap`.
  - Unfortunately, comments are not preserved, so this still leads to
    some data loss. See: dpranke/pyjson5#28

Closes mechatroner#39
@parasyte parasyte force-pushed the fix/syntax-and-colorscheme branch from d6ffb76 to b11ea9e Compare August 28, 2024 00:36
@mechatroner
Copy link
Owner

Thank you very much for this contribution!

But I can't merge the json5 part of the PR - in my opinion, this dependency significantly increases the complexity and the code base size and it is not a sustainable approach in the long term.

I would appreciate it if you could split it into smaller parts. I can also still try to merge this PR piecewise (not sure how to do it yet) when I have a relatively free evening.

@parasyte
Copy link
Contributor Author

Aside from the json5 dependency (and updating all of the pregenerated syntax grammars) this is already a very small patch. Four changed files, excluding the automated parts.

I agree that the dependency is very unfortunate. On the other hand, it might be better than writing a bespoke or in situ parser! I used git subrepo so that maintaining it is trivial. And it exists in its own commit, making it a simple matter to revert.

The other other option is using a SublimeText/PackageControl dependency. Since there are no json5-compatible parsers currently available as a PackageControl dependency (and I don't want to maintain one myself) vendoring was the least disruptive option.

It comes with another downside, though, and that is mentioned in the commit comment that uses it: The dependency is inadequate because it does not preserve comments. I just found https://pypi.org/project/json-five/ which could fix that problem. But a decision still needs to be made between either vendoring it or publishing a dependency on PackageControl.

I think I've enumerated all three options available for fixing #39 and related problems described in the b11ea9e commit comment. Using a PackageControl dependency will certainly look the smallest from the PR perspective. I'm unfamiliar with how it all works, only noting that I came across the linked documentation that describes the process. (Seems like kind of a hack, to me. But if it's the best way to depend on non-trivial code, maybe it's worth the effort?)

@michaelblyons
Copy link

What is JSON5 needed for? Is there something that ST's settings.get doesn't work for?

@mechatroner
Copy link
Owner

mechatroner commented Sep 2, 2024

I cherry-picked Fix CSV file extensions and syntax scopes and published a new release: https://github.com/mechatroner/sublime_rainbow_csv/releases/tag/2.10.0. Thanks again for contributing your time to work on this and other issues. We need to find a way to adjust Fix custom color scheme CL to avoid external dependencies. Perhaps settings.get proposed by @michaelblyons would do the trick.

@parasyte
Copy link
Contributor Author

parasyte commented Sep 2, 2024

What is JSON5 needed for? Is there something that ST's settings.get doesn't work for?

My understanding is that the settings API doesn’t give you a way to specify which file you are manipulating. (If I’m wrong, it would be a great improvement to remove the dependency!)

@FichteFoll
Copy link

My understanding is that the settings API doesn’t give you a way to specify which file you are manipulating.

That is kind of correct. The API only accepts a basename and stores any modifications you do inside a corresponding file in the User package.

That said, that's exactly what the code is doing anyway:

https://github.com/parasyte/sublime_rainbow_csv/blob/b11ea9e8fdb46cc5ce3f026f01e9cbbc5fd01e24/main.py#L427

And, even that aside, I don't see why you couldn't just use the built-in json library to write to the file because, at least as far as I can see, the json5 package doesn't mention anything about preserving existing comments or style via a roundtrip parser (like ruamel.yaml).

@parasyte
Copy link
Contributor Author

parasyte commented Sep 3, 2024

I understand that the path is easy to construct, but it's unclear how the settings API is supposed to use it, if it can at all.

The json5 package does not preserve comments (one of the limitations that I documented) but the json-five package does. The json package that ships with Python is not adequate because it cannot parse comments or trailing commas, both of which are common in the Sublime settings files.

@FichteFoll
Copy link

If this just concerns parsing, you can use sublime.decode_value.

@FichteFoll
Copy link

Still, I'd like to understand why you couldn't use ST's settings API to modify settings. Perhaps there is a misunderstanding here. The general usage would be as follows:

syntax_settings = sublime.load_settings(syntax_file_basename)
syntax_settings.set('color_scheme', COLOR_SCHEME_FILE)
sublime.save_settings(syntax_file_basename)

@parasyte
Copy link
Contributor Author

parasyte commented Sep 3, 2024

The answer is that I'm just unfamiliar with the API. If those three lines work, then that's the obvious solution.

@parasyte
Copy link
Contributor Author

parasyte commented Sep 3, 2024

And if it is that simple, why was the original code overwriting the settings with a hardcoded string? When I had trouble deciphering the documentation (why is the returned Syntax object completely disconnected and unreferenced by the sublime.save_settings() call?) I assumed that the reason for not using it in the first place was because it wasn't fit for purpose.

@FichteFoll
Copy link

I cannot answer this question since I was mainly made aware of this PR via a question on discord, but since the API usage I mentioned hasn't changed since at least ST2 days, I can only assume it was either because there was a weird edge case that we're not thinking of here or because the option was simply not considered (perhaps also due to unfamiliarity with the API).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom colors Make generated syntaxes a sub-flavor of text.csv
4 participants