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

Migrate language modes to CSV #1528

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

FireChickenProductivity
Copy link
Contributor

No description provided.

core/user_settings.py Outdated Show resolved Hide resolved
core/user_settings.py Show resolved Hide resolved
@FireChickenProductivity FireChickenProductivity marked this pull request as ready for review August 5, 2024 17:37
core/modes/language_modes.py Outdated Show resolved Hide resolved
core/modes/language_modes.py Outdated Show resolved Hide resolved
core/modes/language_modes.py Outdated Show resolved Hide resolved
core/modes/language_modes.py Show resolved Hide resolved
@FireChickenProductivity
Copy link
Contributor Author

This is ready for review again.

@FireChickenProductivity
Copy link
Contributor Author

Snippets apparently uses language_ids from language_modes.py, so it might be worth making sure that the snippets system still works.

The following code for dealing with snippets will not get reran on updating the CSV.

# Create a context for each defined language
for lang in language_ids:
    ctx = Context()
    ctx.matches = f"code.language: {lang}"
    context_map[lang] = ctx

I could address this by creating a callback registration mechanism inside language_modes.py.

This list is alphabetized.
@nriley
Copy link
Collaborator

nriley commented Aug 10, 2024

Snippets apparently uses language_ids from language_modes.py, so it might be worth making sure that the snippets system still works.

The following code for dealing with snippets will not get reran on updating the CSV.

# Create a context for each defined language
for lang in language_ids:
    ctx = Context()
    ctx.matches = f"code.language: {lang}"
    context_map[lang] = ctx

I could address this by creating a callback registration mechanism inside language_modes.py.

We could declare an action that returns the list of languages?

@FireChickenProductivity
Copy link
Contributor Author

FireChickenProductivity commented Aug 10, 2024

We could declare an action that returns the list of languages?

Yes, but that would not cause the snippets code to update in response to the csv updating.

@nriley
Copy link
Collaborator

nriley commented Aug 10, 2024

We could declare an action that returns the list of languages?

Yes, but that would not cause the snippets code to update in response to the csv updating.

Gotcha. Not sure if we have an example of this (I couldn't find one on a cursory search of community), but an action that triggers when the list is updated seems reasonable.

@FireChickenProductivity
Copy link
Contributor Author

We could declare an action that returns the list of languages?

Yes, but that would not cause the snippets code to update in response to the csv updating.

Gotcha. Not sure if we have an example of this (I couldn't find one on a cursory search of community), but an action that triggers when the list is updated seems reasonable.

I plan on writing code to allow registering callbacks to handle language mode updates.

@FireChickenProductivity
Copy link
Contributor Author

I theoretically have the code working, but I do not use the snippets system, so I will need to learn how it works before I can test it properly.

@FireChickenProductivity
Copy link
Contributor Author

I did further testing of the snippet system, and it seems to be working with my changes. I consider the pull request ready for review, but someone more familiar with the snippet system than myself should make sure that my changes did not break anything.

@AndreasArvidsson
Copy link
Collaborator

Notes form their community session. We're not sure that introducing a event system especially for this use case is desirable.
I will have a look and see if I can get rid of the dependency on language ids from the snippets. Let's talk more about this on the next meetup.
If necessary another developer can finish this.

@FireChickenProductivity
Copy link
Contributor Author

I am going to be busy for a while starting next week, so I am fine with another developer taking this PR.

@AndreasArvidsson
Copy link
Collaborator

Okay I looked into the snippet dependency on language ids and the result is not that good. Basically the only reason why I'm using the language ids from the language modes file and not the actual snippets themselves is because of performance. Dynamically creating contexts is problematic for Talon. Even to the extent that it have been suggested we should remove the custom snippet dir setting.

The current implementation creates all the contexts once before Talon is started/ready

# Create a context for each defined language
for lang in language_ids:
ctx = Context()
ctx.matches = f"code.language: {lang}"
context_map[lang] = ctx

Issue talking about the problem with updating snippet contexts
#1373

The implementation in this pull request removes the performance gain in creating the contexts before Talon starts. In that case we could just get the language ids from the snippet files themself. I can't really say how much of a problem it is to create the context dynamically, but I definitely feel that if we are going to create them dynamically we should not do it based upon the language mode file, but the snippet files themself.


# Create a context for each defined language
for lang in language_ids:
ctx = Context()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to keep this implementation we definitely should check if the context already exists before creating a new one

@AndreasArvidsson
Copy link
Collaborator

AndreasArvidsson commented Aug 24, 2024

My preferred solution would be to get the language identifiers from the snippet files themselves directly when the file is read so the context would be created before Talon is ready. The only scenario where we would dynamically create a context would be if you add a new snippet language which hopefully is not that common. The problem with this approach is that we have a setting for custom snippet dir. This setting is not available until Talon is ready. The implementation in this pull request might be the the best solution available at the moment since the language csv file can be read immediately at start.

make_sure_settings_file_exists()


@resource.watch(settings_filepath)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this handle if the file doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last time I tested the code, it created the file if it did not exist.

@knausj85
Copy link
Member

knausj85 commented Aug 31, 2024

Okay I looked into the snippet dependency on language ids and the result is not that good. Basically the only reason why I'm using the language ids from the language modes file and not the actual snippets themselves is because of performance. Dynamically creating contexts is problematic for Talon. Even to the extent that it have been suggested we should remove the custom snippet dir setting.

The current implementation creates all the contexts once before Talon is started/ready

# Create a context for each defined language
for lang in language_ids:
ctx = Context()
ctx.matches = f"code.language: {lang}"
context_map[lang] = ctx

Issue talking about the problem with updating snippet contexts #1373

The implementation in this pull request removes the performance gain in creating the contexts before Talon starts. In that case we could just get the language ids from the snippet files themself. I can't really say how much of a problem it is to create the context dynamically, but I definitely feel that if we are going to create them dynamically we should not do it based upon the language mode file, but the snippet files themself.

To me, it sounds like we might be better off keeping the language ids in python. Maybe we should instead clean up the extension definition stuff e.g. using a data class to define the list of extensions/associated spoken forms to make it more easily understandable/modifiable, and live with the need to branch this particular file for performance reasons. Plus, the alternative solutions to this PR require a talon restart if we modify the file, which is downer.

@AndreasArvidsson
Copy link
Collaborator

In the community session today we talked further on this topic. We think we should probably have a new python file code_languages.py where we have the language definitions. This python file can then be used by the language modes and the snippets.

We want to have these language definitions typed and something like this could probably work:

string_or_list = str | list[str]
lang = tuple[str, string_or_list] | tuple[str, string_or_list, string_or_list]

languages: list[lang] = [
    ("python", ["python"]),
    ("java", "java"),
    ("javascript", ["javascript"], ["something something"]),
]

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.

4 participants