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

Improve README with ftdetect gotcha #62

Closed
wants to merge 3 commits into from

Conversation

ixti
Copy link
Member

@ixti ixti commented Aug 25, 2016

Copied logic from vim-coffee, so all creds for actual work goes there ;)
@rapind
Copy link

rapind commented Aug 26, 2016

Interesting to note that we had set filetype=slim in master a couple years ago, but it was changed back for some reason (not sure why). See: 1ac9ebd

@ixti
Copy link
Member Author

ixti commented Aug 29, 2016

@igas @rapind What do you think about the PR now?

@ixti
Copy link
Member Author

ixti commented Aug 29, 2016

Instead of brute forcing filetype, I think we should just inform users of the issue and how to fix it.

@ixti ixti force-pushed the fix/ftdetect branch 2 times, most recently from ed2c097 to 52a015d Compare August 29, 2016 17:24
And possible ways to fix that
@rapind
Copy link

rapind commented Aug 30, 2016

@ixti I definitely think this is an improvement. It's kind of a strange use case that has you scratching your head, so documenting it is definitely nice for user's that hit it.

Known Issues
------------

We use `setfiletype` upon autodetect, which does not overrides filetype once it
Copy link

Choose a reason for hiding this comment

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

did you mean which overrides previous values instead of which does not overrides?

Copy link

Choose a reason for hiding this comment

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

set fieltype does override. setf does not.

@ixti ixti changed the title Improve file detection Improve README with ftdetect gotcha Aug 30, 2016
@ixti
Copy link
Member Author

ixti commented Aug 30, 2016

@rapind although I understand your point of view (and I'm actually gonna use set filetype=slim in my .vimrc). I think there is a pretty small but still possible chance of causing issue for some potential user of this plugin if we will go obtrusive way.

And personally I think that any plugin should do as little as possible by default. So that user would configure/enable necessary behavior himself as he needs.

@rapind
Copy link

rapind commented Aug 30, 2016

I've also changed it locally. I just think it'll impact more people by being unobtrusive, and I haven't been able to think of a scenario where being obtrusive would impact anyone (why would they assign something else to .slim?).

I'm happy enough with a mention in the readme though. We've probably discussed it to death at this point :)

@ixti
Copy link
Member Author

ixti commented Aug 30, 2016

@rapind I can't recall at the moment but I remember I saw couple of absolutely different projects using same filename extensions for their files.

@ixti
Copy link
Member Author

ixti commented Sep 5, 2016

@minad Please merge this, if you have no objections.

@ixti
Copy link
Member Author

ixti commented Sep 5, 2016

This PR does not changes current behavior, but adds README description of it and how to change to be more "enforced".

@minad
Copy link
Member

minad commented Sep 6, 2016

@ixti @genki @rapind I cannot really judge this and it would be good if we have someone maintaining the vim scripts. Is any one of you willing to do so?

@ixti
Copy link
Member Author

ixti commented Sep 6, 2016

@minad I'm not proficient in VimL to call myself a good candidate to be a maintainer, but I'm definitely will be happy to help. In any case, this particular change is not adding changes to the vim script itself ;)) it's adding explanation to the README why current version does not work in a very specific situation as one might expect, and how to avoid this.

@rapind
Copy link

rapind commented Sep 6, 2016

@minad I've literally never even touched VimL. At most just tweaked my own configs. I don't even get a chance to use slim much these days either :(

@ixti
Copy link
Member Author

ixti commented Sep 27, 2016

Closing this due to no activity (I assume no interest as well) from maintainers side.

@ixti ixti closed this Sep 27, 2016
@ixti ixti deleted the fix/ftdetect branch September 27, 2016 15:54
@minad
Copy link
Member

minad commented Sep 27, 2016

@ixti As I wrote above, there is a need of someone maintaining this. Even if the maintainers are not very proficient in Viml, they could still take care that the PRs get tested and merged...

ixti added a commit that referenced this pull request Sep 27, 2016
And possible ways to fix that

See: #62
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.

3 participants