Improve clarity of dotfile edits #4054

ghost · 2017-05-31T15:00:20Z

The RVM installer edits several dot-files (and creates them, if they do not already exist). It can be confusing for a user to find dot-files in their home directory for shells they have never installed (e.g. .mkshrc, if they are not a mksh user). It can also be hard for a novice user to know which edits to their dot-files were made by what, when, and why.

This PR ~~fixes~~ reduces that confusion, by noting, for ~~each edit~~ some edits:

that the RVM installer made the edit
when the RVM installer made the edit
the extent of the edit, using octothorpes as delimiters. Layout loosely inspired by examples such as:
- https://github.com/syl20bnr/spacemacs/blob/master/core/templates/.spacemacs.template
- https://github.com/nicksp/dotfiles/blob/master/shell/bash_profile

The other edits made to those files by the installer (e.g. by lines like printf "%b" "\ntest -f ${etc_profile_file} && source ${etc_profile_file}\n" > $zshrc_file) will require additional work (e.g. in a separate pull request, or as additions to this one) to indicate as having been made by the installer. I don't have time to address all of those at the moment, but it would be good for RVM to have these improvements made at some point.

The RVM installer edits several dot-files (and creates them, if they do not already exist). It can be confusing for a user to find dot-files in their home directory for shells they have never installed (e.g. .mkshrc, if they are not a mksh user). It can also be hard for a novice user to know which edits to their dot-files were made by what, when, and why. This commit fixes that confusion, by noting, for each edit: - that the RVM installer made the edit - when the RVM installer made the edit - the extent of the edit, using octothorpes as delimiters. Layout loosely inspired by examples such as: https://github.com/syl20bnr/spacemacs/blob/master/core/templates/.spacemacs.template https://github.com/nicksp/dotfiles/blob/master/shell/bash_profile

pkuczynski · 2017-05-31T15:05:25Z

This is only for ~/.bash_profile. How about the other files we modify?

pkuczynski · 2017-05-31T15:06:34Z

scripts/functions/installer

@@ -1100,8 +1100,15 @@ setup_user_profile_rc()
    do
      touch "$profile_file"
      printf "%b" "
-# Add RVM to PATH for scripting. Make sure this is the last PATH variable change.
+###############################################################################
+# At $(date --iso-8601=s), the RVM installer added this block (and also created


I would probably add RVM version in here too: RVM 1.29.1 installer...

I would probably add RVM version in here too: RVM 1.29.1 installer

Sounds good. Will push another commit shortly.

pkuczynski · 2017-05-31T15:08:33Z

scripts/functions/installer

+# this file if it did not already exist).
+#########
+#
+# Add RVM to PATH for scripting. Ensure this is the last PATH variable change.


This line could be written in a more clear way, don't you think? Maybe "Include RVM in the PATH. Ensure it is always at the end of the PATH".

Agreed. Will push shortly.

ghost · 2017-05-31T15:09:13Z

@pkuczynski wrote:

This is only for ~/.bash_profile.

I don't think so. It should apply to all dotfiles in the search_list.

pkuczynski · 2017-05-31T15:10:59Z

Indeed, had a look on the too small part of the file, sorry...

pkuczynski · 2017-05-31T15:11:12Z

Waiting for @mpapis review before merge...

As suggested here: #4054 (comment)

As proposed here: #4054 (comment)

pkuczynski · 2017-05-31T15:30:22Z

scripts/functions/installer

-# this file if it did not already exist).
+# At $(date --iso-8601=s), this code block (and also this file, if it did not
+# already exist) was created by:
+# $__rvm_version


I somehow liked more when it said "RVM 1.29 installed" :)

I agree :) I couldn't immediately see an easy, robust way to achieve that, though. If you know a better mechanism than using $__rvm_version, let me know :)

Did you try ${rvm_version}?

If that does not work we might need to wait until my PR #4043 is reviewed by @mpapis and merged.

Thanks. Edit pushed.

Per #4054 (comment)

pkuczynski · 2017-05-31T19:13:40Z

scripts/functions/installer

@@ -1100,8 +1100,15 @@ setup_user_profile_rc()
    do
      touch "$profile_file"
      printf "%b" "
-# Add RVM to PATH for scripting. Make sure this is the last PATH variable change.
+###############################################################################
+# At $(date --iso-8601=s), the RVM ${rvm_version} created


Have you tried if that works?

Not specifically. Any reason why it wouldn't?

$ echo $rvm_version $ rvm_version='1.2.3' $ echo "At $(date --iso-8601=s), the RVM Installer from RVM ${rvm_version} created" At 2017-05-31T20:49:11+0100, the RVM Installer from RVM 1.2.3 created

I'm afraid I'm not familiar with these steps that would seem necessary for fully testing:

Uninstalling RVM (probably easy, but maybe time-consuming)

Installing RVM from the branch this PR is based on (trickier)

I'm afraid I can't really put any more time into the matter at the moment either, sorry :( I hope that what I've contributed so far is helpful and can make it into RVM once it has been tested/approved/tweaked by you and @mpapis .

Thanks for maintaining RVM and for your great feedback on this PR :)

pkuczynski · 2017-06-15T14:47:46Z

@mpapis any feedback on this?

mpapis

I like it, should we add the same message in other places like when we add source lines?

mpapis · 2017-06-16T11:26:04Z

scripts/functions/installer

+###############################################################################
+# At $(date --iso-8601=s), the RVM ${rvm_version} created
+# this code block (and also this file, if it did not already exist).
+#########


no need for this separator :)

The idea with the separators is this:

########################### <- First long separator indicates start of hunk inserted by rvm # rvm should put explanation here ### <- Short separator visually separates end of explanation from start of script snippet rvm should put snippet here ########################### <- Second long separator indicates end of hunk inserted by rvm

I think it's ok to have the short separator.

pkuczynski · 2017-06-16T14:47:42Z

@mpapis in which other places you would like something like this to be added?

mpapis · 2017-06-16T21:15:59Z

scripts/functions/installer in the setup_user_profile_login just after rvm_out "Adding rvm loading line to ${target_login[*]}."

there are other implications, when we run with --auto-dotfiles it will accumulate the comments, please try: ./install --auto-dotfiles to check it locally.

pkuczynski · 2017-06-16T23:24:16Z

Indeed multiple runs duplicate the info entry. I am working on a solution for this issue.

pkuczynski

Work in progress...

pkuczynski · 2017-07-02T11:59:49Z

@mpapis this is finally done I believe! I reversed some of the changes to ensure backward compatibility with the entries added by the previous versions of RVM. Also refactored and simplified some code.

From my point of view its ready for merge. Unless you want to veto it?

mpapis · 2017-07-03T20:41:40Z

scripts/functions/installer

@@ -1019,150 +1021,175 @@ setup_user_profile_check()
 setup_user_profile_detect()
 {
  etc_profile_file="/etc/profile.d/rvm.sh"
-  search_list_mksh=( "$HOME/.mkshrc" "$HOME/.profile" )
-  search_list_bash=( "$HOME/.bashrc" "$HOME/.bash_profile" "$HOME/.bash_login" )
-  search_list_zsh=( "${ZDOTDIR:-${HOME}}/.zshenv" "${ZDOTDIR:-${HOME}}/.zprofile" "${ZDOTDIR:-${HOME}}/.zshrc" "${ZDOTDIR:-${HOME}}/.zlogin" )


Any particular reason for limiting the list, my idea with all the files was that user can have sourcing/PATH line in any of them as long as it solves their needs, then the code was detecting which entries were already set up and not installing needlessly in other file which we think is more appropriate for it.

The different list was used for searching and different for adding. But then they were merged anyway. So I simplified this and use only one list now.

but I can not find list of the previous files :(

In newer UNIXes aren't used anymore

mpapis · 2017-07-03T20:44:26Z

scripts/functions/installer

-  setup_user_profile_rc
-  setup_user_profile_login
+  if
+    (( rvm_auto_dotfiles_flag == 1 ))


Does it mean that we will not add the sourcing/PATH lines automatically on fresh install if user does not pass --auto-dotfiles flag? This can increase amount of dissatisfied users with not working setup because of missing this lines.

Previously condition for adding was:

if (( rvm_auto_dotfiles_flag == 1 || ${#found_rc[@]} == 0 ))

so I felt that we are not making a change in functionality, but you are right. It's mistake. I will remove the rvm_auto_dotfiles_flag as in the new code we re-add the lines on every install, as I think it simplifies the code a lot.

if we keep updating the line at every install it might mess up things? what if user adds something that depends on RVM and we move our code after his addition?

mpapis · 2017-07-03T20:45:26Z

scripts/functions/installer

      then
-        found_login+=( "$profile_file" )
+        rvm_debug "Removing RVM info line from ${file}"
+        __rvm_sed_i "$file" -e '/RVM BEGIN$/,/RVM END$/d'


saizai · 2018-04-14T07:41:27Z

One note: I've occasionally found duplicate rvm lines in dotfiles. Would be nice if it cleaned up old ones, or if not doing differently just tweaked them with a "rechecked at date X, nothing to change since added at date Y".

Sam Pablo Kuper added 2 commits May 31, 2017 15:55

Shorten excessively long line

b3609e2

pkuczynski requested review from mpapis and pkuczynski May 31, 2017 15:05

pkuczynski reviewed May 31, 2017

View reviewed changes

Sam Pablo Kuper added 2 commits May 31, 2017 16:13

Add RVM version to dotfiles

fa8654f

As suggested here: #4054 (comment)

Improve dotfile comment

174a4ee

As proposed here: #4054 (comment)

pkuczynski reviewed May 31, 2017

View reviewed changes

Sam Pablo Kuper and others added 2 commits May 31, 2017 18:47

Use rvm_version variable

a1c694f

Per #4054 (comment)

Update installer

f898e94

pkuczynski reviewed May 31, 2017

View reviewed changes

Merge branch 'master' into feature/clearer-dotfiles

b99689d

mpapis reviewed Jun 16, 2017

View reviewed changes

Merge branch 'master' into feature/clearer-dotfiles

d36e345

pkuczynski approved these changes Jun 16, 2017

View reviewed changes

Fix fetching current date

ff865e2

pkuczynski requested changes Jun 16, 2017

View reviewed changes

pkuczynski added 4 commits June 19, 2017 00:37

Prepend all injected lines with RVM info

d2e00f3

Merge branch 'master' into feature/clearer-dotfiles

06146ad

Improve regexp for detection

422fde0

Merge branch 'master' into feature/clearer-dotfiles

b1d44df

pkuczynski changed the title ~~Give clarity to code block installed to dot-files~~ Improve clarity of dotfile edits Jul 2, 2017

Final version with simplified code and properly removal of older entries

6903608

pkuczynski previously approved these changes Jul 2, 2017

View reviewed changes

pkuczynski added this to the rvm-1.29.3 milestone Jul 2, 2017

pkuczynski added the feature label Jul 2, 2017

mpapis reviewed Jul 3, 2017

View reviewed changes

pkuczynski added 2 commits July 4, 2017 12:23

Merge branch 'master' into feature/clearer-dotfiles

1e705ae

Update installer

7ca3aae

pkuczynski modified the milestones: rvm-1.29.3, rvm-1.29.4 Sep 14, 2017

pkuczynski modified the milestones: rvm-1.29.4, rvm-1.29.5 Sep 23, 2018

pkuczynski modified the milestones: rvm-1.29.5, rvm-1.29.7 Dec 12, 2018

pkuczynski modified the milestones: rvm-1.29.7, rvm-1.29.8 Jan 3, 2019

pkuczynski modified the milestones: rvm-1.29.8, rvm-1.29.9 May 8, 2019

merceroncode approved these changes Jul 8, 2019

View reviewed changes

pkuczynski modified the milestones: rvm-1.29.9, rvm-1.29.10 Jul 10, 2019

pkuczynski modified the milestones: rvm-1.29.10, rvm-1.29.11 Mar 25, 2020

pkuczynski modified the milestones: rvm-1.29.11, rvm-1.29.12 Dec 29, 2020

pkuczynski modified the milestones: rvm-1.29.12, rvm-1.29.13 Jan 15, 2021

pkuczynski dismissed their stale review via 7ca3aae July 26, 2021 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve clarity of dotfile edits #4054

Improve clarity of dotfile edits #4054

ghost commented May 31, 2017 •

edited by pkuczynski

pkuczynski commented May 31, 2017

pkuczynski May 31, 2017

ghost May 31, 2017

pkuczynski May 31, 2017

ghost May 31, 2017

ghost commented May 31, 2017 •

edited by ghost

pkuczynski commented May 31, 2017

pkuczynski commented May 31, 2017

pkuczynski May 31, 2017

ghost May 31, 2017

pkuczynski May 31, 2017

ghost May 31, 2017

pkuczynski May 31, 2017

ghost May 31, 2017

ghost May 31, 2017

pkuczynski commented Jun 15, 2017

mpapis left a comment

mpapis Jun 16, 2017

ghost Jun 16, 2017

pkuczynski Jun 16, 2017

pkuczynski commented Jun 16, 2017

mpapis commented Jun 16, 2017 •

edited

pkuczynski commented Jun 16, 2017

pkuczynski left a comment

pkuczynski commented Jul 2, 2017 •

edited

mpapis Jul 3, 2017

pkuczynski Jul 4, 2017

mpapis Jul 4, 2017

merceroncode Jul 8, 2019

mpapis Jul 3, 2017

pkuczynski Jul 4, 2017

mpapis Jul 4, 2017

mpapis Jul 3, 2017

saizai commented Apr 14, 2018

Improve clarity of dotfile edits #4054

Are you sure you want to change the base?

Improve clarity of dotfile edits #4054

Conversation

ghost commented May 31, 2017 • edited by pkuczynski

pkuczynski commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 31, 2017 • edited by ghost

pkuczynski commented May 31, 2017

pkuczynski commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkuczynski commented Jun 15, 2017

mpapis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkuczynski commented Jun 16, 2017

mpapis commented Jun 16, 2017 • edited

pkuczynski commented Jun 16, 2017

pkuczynski left a comment

Choose a reason for hiding this comment

pkuczynski commented Jul 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saizai commented Apr 14, 2018

ghost commented May 31, 2017 •

edited by pkuczynski

ghost commented May 31, 2017 •

edited by ghost

mpapis commented Jun 16, 2017 •

edited

pkuczynski commented Jul 2, 2017 •

edited