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(config): fetch configs if user has configured (#66) #67

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

kevinhwang91
Copy link
Contributor

No description provided.

@danymat
Copy link
Owner

danymat commented Feb 10, 2022

What is the purpose of the meta table then ?

@kevinhwang91
Copy link
Contributor Author

Defer load. If I only use Lua filetype, I don't want to load all of the filetype configurations.

@danymat
Copy link
Owner

danymat commented Feb 10, 2022

I tried to print inside it, but it seems that it is never called

@kevinhwang91
Copy link
Contributor Author

Remove your filetype configuration and print again.

@danymat
Copy link
Owner

danymat commented Feb 10, 2022

It seems to work, however, the get_template() still not work because conf.languages is only holding added languages information.

To test: print(vim.inspect(require('neogen').get_template("lua')))"

@kevinhwang91
Copy link
Contributor Author

No, it will work, check out your config.

@danymat
Copy link
Owner

danymat commented Feb 10, 2022

Hmm, i don't get it then. conf.languages is not resulting to anything, for example ruby is broken because in config.setup, we only loop over custom setup

@danymat
Copy link
Owner

danymat commented Feb 10, 2022

Okay after digging a little bit, I found out that modifying the template with the template API, for example:

require("neogen").get_template("python"):config({ annotation_convention = "numpydoc" })
Is not going to work, I guess because the table is generated from the __call, and the reference is not kept with _data

@kevinhwang91
Copy link
Contributor Author

It's not related to this issue. template.config just extends the table and doesn't assign the value back to conf.languages[ft].template is holding.

@danymat danymat merged commit c4b5638 into danymat:main Feb 10, 2022
@kevinhwang91 kevinhwang91 deleted the fix-missing-user-conf branch February 10, 2022 16:59
@danymat
Copy link
Owner

danymat commented Feb 10, 2022

I'm kinda scratching my head right now, because of new behavior. Let me explain:

  • Before merging refactoring, the neogen template api (:h neogen-template-api) did succeed in updating a configuration even if there was no merging table back to config (as you mentioned here: fix(config): fetch configs if user has configured (#66) #67 (comment)), because the reference to the table was somehow the same.
  • Now that the refactor gets the configuration for a language with an other way, the template api is not working anymore if we want to modify the template

That means we have to decide:

  • keep the api and try to fix this, by somehow "saving" the new content back to the configuration table
  • Delete the API (or keep it private only for setting up the languages). I don't really like this way, but it's a way of doing things.

Any thoughts @kevinhwang91 ?

@kevinhwang91
Copy link
Contributor Author

That means we have to decide:

  1. keep the api and try to fix this, by somehow "saving" the new content back to the configuration table
  2. Delete the API (or keep it private only for setting up the languages). I don't really like this way, but it's a way of doing things.

I suggest you delete the API. Users already have setup function to change the config to do what they want.
Assume one day, a user posts an issue and you found s/he changed neogen's configuration inside multiple files or functions (via API and setup function), how could s/he easy to reproduce the issue?
More API more scratching, you can't easy refactor the code base and the code will be rot in the future. TBH, neogen is not a lib and If you want to maintain this plugin easy and happy, you should close the redundant API and keep the plugin stable and simple. For me setup function is enough to customize neogen.

FYI, I have two Lua plugins in maintenance, they are easy to maintain because of less APIs or options.

@danymat
Copy link
Owner

danymat commented Feb 11, 2022

Thanks for the feedback, I pushed a new version to make this API private

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.

2 participants