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

Use with-temp-file to write the statistics file #8

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@lunaryorn

lunaryorn commented Sep 13, 2016

Avoids a couple of undesired side effects of "write-file".

Use with-temp-file to write the statistics file
Avoids a couple of undesired side effects of "write-file".
@ilohmar

This comment has been minimized.

Contributor

ilohmar commented Sep 13, 2016

Hi, thanks for the PR! Please note that this is a GNU ELPA package, so each contributor needs to assign copyright to the FSF. I followed a flycheck-related discussion (thanks for that package, BTW!) on that issue, and IIRC, you did/do not want to make that assignment.

Now, as this is only a small change, it would be fine without the assignment. And I will gladly merge it iff you are ok with the situation (effectively disclaiming your copyright on the patch). I would not be able to accept any further PRs, however. WDYT?

@dgutov

This comment has been minimized.

Member

dgutov commented Sep 13, 2016

Now, as this is only a small change, it would be fine without the assignment.

I think that's only the case if Sebastian hasn't yet exhausted his total 15-line (or so) limit of total Emacs core contributions.

On the other hand, whether a contributor agrees to "disclaim" the copyright in this case, is largely irrelevant.

@ilohmar

This comment has been minimized.

Contributor

ilohmar commented Sep 13, 2016

Oops, I did not think this through. Yeah, I guess the limit applies to the whole contribution to Emacs core plus ELPA.

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 13, 2016

I wasn't aware. As I'm beyond that limit already and will not sign a copyright agreement with the FSF I have not choice but to close this pull request. I'm sorry to have wasted your time.

@lunaryorn lunaryorn closed this Sep 14, 2016

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

That said you should change the way you write the file nonetheless. I'm sorry that I'm unable to help you anymore.

@dgutov

This comment has been minimized.

Member

dgutov commented Sep 14, 2016

Would using write-region be better?

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

@dgutov with-temp-file is just with-temp-buffer + write-region.

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

And the less you use write-region the better—it's one of the worst functions of Emacs, API-wise. 😉 It's interface is a mess…

@dgutov

This comment has been minimized.

Member

dgutov commented Sep 14, 2016

Well, I think it's fine to use here, if it avoids the problems you referred to (but hadn't actually mentioned).

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

@dgutov Oh, sorry, I thought they'd be known. write-file calls set-visited-file-name which switches the major mode in the default configuration of Emacs—i.e. does the set-auto-mode/hack-local-variables dance—which triggers all sorts of unpredictable hooks and side effects, like prompts for unsafe local variables, or minor modes—as in my case, Flycheck.

I noticed the temporary files that Flycheck creates when checking Emacs Lisp, i.e. every time company statistics saved the file, a company-statistics_flycheck.el file would briefly appear in Magit. IOW, Flycheck checked the statistics file every time it was saved, because of write-file.

You should never use interactive functions, i.e. find-file, write-file, save-buffer, etc. to read or write data files.

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

@dgutov It's fine to use write-region here but you're essentially writing with-temp-file yourself then. If that's what it takes to make the FSF happy go ahead.

@dgutov

This comment has been minimized.

Member

dgutov commented Sep 14, 2016

Thanks!

It's fine to use write-region here but you're essentially writing with-temp-file yourself then. If that's what it takes to make the FSF happy

Indeed.

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

So just by posting this patch I've already spoiled the proper with-temp-file solution?

@dgutov

This comment has been minimized.

Member

dgutov commented Sep 14, 2016

That's my understanding, yes.

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

Oh dear, that's insane. I should seriously start trolling GNU Emacs by posting patches for bugs so that they can't fix them…

@dgutov

This comment has been minimized.

Member

dgutov commented Sep 14, 2016

Please take it up with the copyright legislators.

@lunaryorn

This comment has been minimized.

lunaryorn commented Sep 14, 2016

Well, that's your problem, not mine.

I'm sorry, I wasn't aware. I'll try to refrain from posting patches to packages in GNU ELPA in future.

@ilohmar

This comment has been minimized.

Contributor

ilohmar commented Sep 14, 2016

No worries, just a somewhat unfortunate consequence. I think Dmitry just wanted to say that if you considered doing anything about the situation (pretty sure you were kidding), the copyright legislators are the ones to blame and address, not the GNU/Emacs people.

I'll reconsider this issue when I have a bit more time --- thanks for bringing my attention to it, anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment