After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 658323 - Deprecate FOLKS_WRITEABLE_STORE in favour of FOLKS_PRIMARY_STORE
Deprecate FOLKS_WRITEABLE_STORE in favour of FOLKS_PRIMARY_STORE
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: libfolks
git master
Other All
: Normal minor
: Future
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-06 07:12 UTC by Philip Withnall
Modified: 2011-09-14 20:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Phase out FOLKS_WRITEABLE_STORE (4.88 KB, patch)
2011-09-08 16:25 UTC, Raul Gutierrez Segales
none Details | Review
Phase out FOLKS_WRITEABLE_STORE (5.47 KB, patch)
2011-09-08 16:30 UTC, Raul Gutierrez Segales
needs-work Details | Review
patch (5.89 KB, patch)
2011-09-14 11:42 UTC, Raul Gutierrez Segales
accepted-commit_now Details | Review

Description Philip Withnall 2011-09-06 07:12:16 UTC
For consistency and clarify of naming, we should prefer to use the FOLKS_PRIMARY_STORE environment variable (though we'll still have to fall back to looking at FOLKS_WRITEABLE_STORE).

(Re. my e-mail of 2011-09-03.)
Comment 1 Raul Gutierrez Segales 2011-09-08 16:25:19 UTC
Created attachment 196009 [details] [review]
Phase out FOLKS_WRITEABLE_STORE

I *strongly* believe that we should not maintain support for FOLKS_WRITEABLE_STORE and just fix whatever we break (if anything, I really doubt packagers or anyone out there could have come to rely on this so quickly). 

The attached patch is a straight replacement.
Comment 2 Raul Gutierrez Segales 2011-09-08 16:30:06 UTC
Created attachment 196010 [details] [review]
Phase out FOLKS_WRITEABLE_STORE

Forgot to update the NEWS file (as usual).
Comment 3 Travis Reitter 2011-09-09 17:04:43 UTC
Review of attachment 196010 [details] [review]:

Let's not make this an API break (I think we should count environment variables in our public interface).

Our breaks in the past have flowed a little too freely, and I don't think it's too heavy to add one extra fall-back check.
Comment 4 Philip Withnall 2011-09-13 17:27:24 UTC
Agreed with Travis. It would be nice if a warning was printed if the user used FOLKS_WRITEABLE_STORE instead of FOLKS_PRIMARY_STORE. This gives us an excuse to remove FOLKS_WRITEABLE_STORE entirely in the future.
Comment 5 Raul Gutierrez Segales 2011-09-14 11:42:10 UTC
Created attachment 196484 [details] [review]
patch

Updated
Comment 6 Philip Withnall 2011-09-14 18:47:15 UTC
Review of attachment 196484 [details] [review]:

Two minor issues, then it looks good to commit.

::: folks/individual-aggregator.vala
@@ +271,1 @@
       var store_config_ids = Environment.get_variable ("FOLKS_WRITEABLE_STORE");

I'd check FOLKS_PRIMARY_STORE first, since we should prioritise it over FOLKS_WRITEABLE_STORE.

@@ +272,3 @@
       if (store_config_ids != null)
+        warning (
+            _("FOLKS_WRITEABLE_STORE is deprecated, use FOLKS_PRIMARY_STORE"));

No need to translate this string; it's a warning which will only ever be seen by developers.
Comment 7 Raul Gutierrez Segales 2011-09-14 20:13:22 UTC
Merged:

commit 152ee3f06bd223c7ed9e9825543554031f490ddf
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Thu Sep 8 17:21:10 2011 +0100

    core: deprecate FOLKS_WRITEABLE_STORE for FOLKS_PRIMARY_STORE
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=658323