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

Remove salt as a dependency #120

Closed
wants to merge 1 commit into from
Closed

Conversation

mew1033
Copy link
Contributor

@mew1033 mew1033 commented Nov 20, 2019

Salt, even when installed via pip, is huge and has a ton of dependencies. All it was being used for was six, and the output coloring. This PR removes salt as a dependency and brings in the relevant code from the salt repo. Thanks to them for using Apache 2!

@roaldnefs roaldnefs added the Status: Invalid This doesn't seem right label Nov 21, 2019
@roaldnefs
Copy link
Member

Thanks @mew1033 for your PR and investing your time to make salt-lint a better tool!

Even though this a great optimization for the current version of salt-lint, we are including salt as a dependency to be able to (partially) render states (in order to fix #19 for example) in future releases. It also allows (custom) rules to use the internals of salt (we could for example use the existing Jinja tokenizer to fix #51).

I'm closing this PR for now, we can always reopen it if we change our mind.

@roaldnefs roaldnefs closed this Nov 21, 2019
@mew1033
Copy link
Contributor Author

mew1033 commented Nov 22, 2019

@roaldnefs That makes a lot of sense. I was looking into ansible-lint and saw how it was using ansible directly.
Do you think it would make sense to create multiple methods of installing salt-lint? Maybe using pip install extras? So if you want a lighter install you can pip install salt-lint, and if you want a full-featured install, you can do pip install salt-lint[salt]?

I just know that, at least for now, I'd much rather have a smaller, lightweight version of salt-lint.

@roaldnefs
Copy link
Member

Do you think it would make sense to create multiple methods of installing salt-lint?

Yes, it would make sense but also adds some complexity to the source code and on the user side during the installation.

I just know that, at least for now, I'd much rather have a smaller, lightweight version of salt-lint.

Would it be an idea to create an issue for this request? If there are more users that are interested in a lightweight variant of salt-lint we can change our priority accordingly. Feel free to refer back to this issue!

@mew1033
Copy link
Contributor Author

mew1033 commented Nov 25, 2019

@roaldnefs Sounds good, I made an issue for this. I'll try to maintain my fork for a while because I personally would much prefer to use salt-lint without salt for now.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants