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

Add Option to Assign Color for Each Commit #220

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sirrow
Copy link

@sirrow sirrow commented Dec 15, 2017

On v.1.4.0, the new feature assigning colors for each author is great.
Furthermore, I want to assign colors for each commit.

@alexcorre
Copy link
Owner

@sirrow thanks for the contribution. is there a way to migrate on first startup peoples preferences so that it translates over to the new enum properly?

@sirrow
Copy link
Author

sirrow commented Jan 5, 2018

@alexcorre I don't make any patches for configuration migration. Also, I have not considered yet how to migrate.
I am worrying which is better: keeping the code simple, or adding migration function. Which do you think better?

@alexcorre
Copy link
Owner

@sirrow sorry for the delay. i think its ok to skip the migration function. A happy medium would be to first read the new enum config value, if its not present fall back to the old config value.

How does that sound?

@sirrow
Copy link
Author

sirrow commented Jan 27, 2018

@alexcorre
I caught the ideas you recommended are

  • If new enum config exists, new enum config is used, and old config is ignored and left in config file.
  • If only old config exists, old config is used

I feel this idea can be implemented a little simpler than migration.
I will try to write some code for this idea soon.

@sirrow
Copy link
Author

sirrow commented Jan 28, 2018

@alexcorre
I tried to implement the idea but it was too hard to implement simply.
Therefore, now , I think it's better to discard old config without any migration.

The reason for retiring implementing the idea is that
atom cannot hide the old config from setting page if atom want to read old config.

I want to hide the old config from settings page because it's function is merged new enum config.
In the other hand, setting for old config should be set in config.js for reading old config by atom config api.
But old config became shown in settings page if setting for old config for reading config is written in config.js.

For reading the old config without showing in setting page, I have to develop another function for reading config for git-blame plugin outside of atom config API. This is too nasty.

In my opinion, for for implementing this config migration cleanly some new function like below is necessary.

  1. Hidden option in config.js is prepared by atom config API.
  2. Migration method like db migration(ex. rake db:migrate) is prepared by atom config API and do the migration when updating plugins.

If someone knows better simple way, please tell me. 😢

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.

2 participants