-
Notifications
You must be signed in to change notification settings - Fork 81
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
a coherent set of config variables/flags #108
Comments
Slowly remembering some of the stuff we talked about, for why |
I put a branch up for this: https://github.com/buildinspace/peru/tree/flags We'll probably need to refactor the docopt parser before we finish this, because throwing all these flags in front of every subcommand doesn't sound like fun. |
Summary: We've been meaning to do this for a while, see #108. Here's the full set: PERU_FILE --peru-file PERU_SYNC_DIR --sync-dir PERU_STATE_DIR --state-dir PERU_CACHE_DIR --cache-dir Note that --peru-file and --sync-dir need to be set together, or not at all; you can't just specify one of them. That's because if you were to set the one that's normally derived from the other (which we could imagine defining in either direction), you end up with weird consequences for the first one that's still derived from the cwd. Test Plan: I need to write new tests for this before I check it in. Reviewers: sean Differential Revision: https://phabricator.buildinspace.com/D210
Summary: We've been meaning to do this for a while, see #108. Here's the full set: PERU_FILE --peru-file PERU_SYNC_DIR --sync-dir PERU_STATE_DIR --state-dir PERU_CACHE_DIR --cache-dir Note that --peru-file and --sync-dir need to be set together, or not at all; you can't just specify one of them. That's because if you were to set the one that's normally derived from the other (which we could imagine defining in either direction), you end up with weird consequences for the first one that's still derived from the cwd. Test Plan: Several new integration tests. In the process, I found out that we had a few cases where we passed a string instead of a tuple to test helper methods. Added a new assertion to catch those. Reviewers: sean Differential Revision: https://phabricator.buildinspace.com/D210
It is very sad to hear you get rid of |
@enzochiau please tell me a little more about how you were using that variable. Were you setting it globally, or calling peru from an alias with it, or what? We added the more explicit |
I set it globally in my login shell as The |
One of the reasons we removed |
Ah ok, I just saw your last comment. Yes, that login shell setting is what I was worried about. The problem is, if you cloned one of my projects and tried to run peru, it would be broken, because we use different file names. Maybe that's not too bad if you're calling peru directly and you can see the error message, but if it's inside a Makefile or something I worry it would be very confusing. |
What I had in mind for your login shell was something like
Then you would just call |
This raises the question: maybe we shouldn't even have created the |
How about the fallback mechanism? For example: Use |
I think even if we do that, we still have most of the downside. Most projects will just use the defaults, so they'll still get broken by the global setting. |
Just to be clear, I'm not worried about you specifically breaking other projects :) What I'm worried about is someone new who's just starting to learn peru (maybe even just starting to learn bash), and the instructions for their project say
The new developer does what they're told and then a week later, when they try to build someone else's random vim plugin, everything is broken, and they have no idea why. That wouldn't really totally be our fault, but it's kind of our fault, because we encouraged projects to use that global setting. So if we can accomplish the same goals without the risk of confusing people, that would be great. This is kind of like how Make has the |
And now that I think about it more, I'm pretty sure we should scrap |
@enzochiau do you think the alias approach will work for you? |
@oconnor663 I think the alias will work for me if I do not run it in subdirectory. I think i can accept that. I agree the global variable is not good. Allow user to change the config may not be a good idea either. It may be very hard to guess what is the special name used in the sub modules When you want to check their configuration. Is it possible to fix the config name as |
It sounds like we should give you a dedicated flag for this use case, which preserves the ability to sync from subdirectories. I just threw up an example diff: https://phabricator.buildinspace.com/D213 @enzochiau and @olson-sean-k, please take a look at that and let me know what you think. I don't think I want the peru file hidden by default. To me it feels more like |
Summary: There is really no good use case for setting these env vars globally. We originally added them to parallel what git does with its own path flags and vars. But I think they're risky for the same reason that PERU_FILE_NAME was risky: they could encourage people to use nonstandard paths for all their projects and then confusingly break projects that aren't theirs. See #108. It's possible there will be some weird cases where users are able to set env vars but not able to pass flags. If we get reports, we can think about adding these back. Reviewers: sean Differential Revision: https://phabricator.buildinspace.com/D211
The code should work for me. |
I think projects should generally avoid custom names for That all being said, I think it's very useful to be able to configure peru to sync into an arbitrary directory using an arbitrary See the discussion in D197. Ultimately, I think the mutually exclusive arguments discussed in that diff are a reasonable choice:
Note that we could support only the |
As an aside, if we support custom names, we may want to call out some of the problems that users may encounter in the docs. For example:
|
@olson-sean-k and I had a discussion in https://phabricator.buildinspace.com/D197, and I'm just copying the result of that here, partly in case we lose the Phabricator server :)
We want to make the following configs available. Each one determines all the ones that come after it, unless those are explicitly set too.
$PERU_FILE
--peru-file
: The filepath to the YAML file to use, instead of searching forperu.yaml
.$PERU_SYNC_DIR
--sync-dir
: The directory where the root of the imports tree goes.$PERU_STATE_DIR
--state-dir
: Where to put stuff that normally goes in the.peru
dir.$PERU_CACHE_DIR
--cache-dir
: Where to put the cache, which normally goes incache/
in the state dir.Note that we are not including a config to change the peru file search name. (We'll be getting rid of the existing
$PERU_FILE_NAME
config.) When peru searches for the file on its own -- which works even if it's somewhere above the current directory -- it will always look for "peru.yaml". It's hard to imagine how changing that in a global config could be useful to anyone, and if you're going to set it locally for some projects, you might as well set the full filepath explicitly.We're also probably going to require that, if either one of
--peru-file
or--sync-dir
are set, then both must be set. @olson-sean-k pointed out some very confusing cases that could result from allowing either one to take a default value based on the other. I've forgotten what they were, but they were bad :)The text was updated successfully, but these errors were encountered: