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 ability to set global packages #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EmmaRamirez
Copy link

This PR:

  • Allows for a very crude form of setting a global package

Some questions:

  • Should it be a specific format? i.e. would json make sense?
  • Should the user be allowed to set multiple globals?
  • What's the most logical way to use globals? Right now, I just have them as defaults if the user doesn't enter anything. But to me, that doesn't feel very intuitive at all.

Sorry if this PR is a bit crude, I just wanted to actually get started with it. Obviously, it'd be awesome if I could suggestions on improving it!

right now it just retrieves the name from a file
@himynameisdave
Copy link
Owner

This is awesome, you did a great job!

giphub

Answers to above questions:

  1. Yes, I think JSON makes the most sense as that's already what's expected when the user is uploading from a package.
  2. I think for now setting one global should be okay, and later if we want we can add in support for many global packages. I could see the use case for wanting many global packages too, like maybe you'd want some for personal projects and some for work projects. For now let's stick to just one global package though, that way we can get this out faster and then iterate on it from there :)
  3. I think this should be the default behaviour, that makes it easier to quickly add your usual default labels, especially if you use the tool on a lot of different projects and always use the same set of labels. But you are right about it not being intuitive, so we we need to think of a better way to let the user know about it 🤔

@Jameskmonger
Copy link
Collaborator

My only feedback is that it doesn't appear to be very clear that it's using the default package. Maybe it should be put as the default value into the text box?

@himynameisdave himynameisdave mentioned this pull request Mar 18, 2017
6 tasks
Copy link
Owner

@himynameisdave himynameisdave left a comment

Choose a reason for hiding this comment

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

@EmmaRamirez A few code review comments, and I just wanted to ask a few more things:

  1. When we prompt the user for the global JSON package, is the expected path relative to the current directory? We should let the user know this.

  2. Once I save my global package, how can I use it? I feel like the "Add Labels From Package" should notify the user that their global package will be used if they don't input anything (but only show this note if they've saved a global package).

  3. We should update the usage section of the readme with the new option 👍

@@ -103,6 +104,20 @@ const addFromPackage = (repo, token, path) => {
.catch(console.warn);
};

const setGlobalPackage = (path, token) => {
fs.writeFile('.git-label-maker-config', pathTool.resolve(__dirname, '..', path), (err) => {
Copy link
Owner

Choose a reason for hiding this comment

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

@EmmaRamirez "labelmaker" should match the name of the repo, like .git-labelmaker-config' 👍

break;

case "add global package":
prompt([{
Copy link
Owner

Choose a reason for hiding this comment

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

You can break off this array for the prompt and put it in the ./bin/prompts/ directory 👍

@himynameisdave himynameisdave modified the milestones: v0.9.1, v0.9.0 Apr 1, 2017
@himynameisdave
Copy link
Owner

@EmmaRamirez are you still working on this? If not I don't mind picking up from where you left off...

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

Successfully merging this pull request may close these issues.

3 participants