APE3: New configuration system#3
Conversation
There was a problem hiding this comment.
commented out to me means they are commented - should we clarify by saying uncommented?
|
If the 'default' configuration from 0.3.0 or earlier is found with no changes, would it make sense to throw it out and replace it with the new one, automatically? Or at least raise a warning that we recommend removing this file? |
|
We should definitely raise a warning recommending removing the file. However, the situation I was hoping to avoid was one where a user moves back and forth between different versions of astropy, but on further reflection, this doesn't really address that either. We may need to include the version in the config file name, so that the two could exist side-by-side. Then, when you don't find one for the current version, you find the next oldest, upgrade that, and write it out as the current version. Then, of course, you may have to update config values in multiple files if you really need to be using different versions of astropy all the time... Anyway, I guess that means the versioning part of this APE requires further thought. |
|
I'll read over this in more detail later... but one quick thing: why APE "4"? Currently there's only 1 and 2, so the next should be "3"...? |
|
I thought the wiki had "allocated" the numbers, but maybe not... |
|
@mdboom - I didn't mean the order to be important for the |
|
Yeah, I think its better if we assign numbers just at the time the PR is issued. So I'd agree that changing this to 3 makes more sense. (I think it's just renaming the file and changing the title of the PR, right? |
|
Renamed to APE3. |
There was a problem hiding this comment.
I think maybe "hinders reproducibility" might be better wording.
|
Regarding the generation of the config file - there is one solution that hasn't really been mentioned which is simply to require users to create and add information to the config file. That is, we never copy any default file to the home directory, but the user can create it there. It means that we can then not worry about having to deal with all the magic to figure out if files have changed, etc. |
|
@astrofrog: I'm not sure never copying the file over actually would resolve much. You still have the problem of understanding/upgrading a user's file from a previous version of astropy if they've created it. As it stands, I'm still proposing that we never replace the user's config file without her explicitly requesting astropy to do so. |
|
Providing a sequence of migration scripts is no biggie, though they do have to be somewhat flexible and fail gracefully on goofy input. But the vast majority of users aren't even likely to know their config file is there. I think an |
|
I've updated a new draft here. It addresses all of the minor points in this PR, and then:
|
|
@mdboom - I agree with all the changes. 👍 from me for this APE. |
There was a problem hiding this comment.
Just to be sure I understand: this version isactually the Astropy release version, right? Not some special version that advances separately from Astropy?
There was a problem hiding this comment.
Yes -- my thinking is that it's the astropy version (excluding any "dev" part, because that will just kill it for us developers).
|
Thanks for putting this all together, @mdboom!
The rest only applies to "platform configuration"
Also, I do agree with @mdboom's point that the configuration should be in the documentation. But I think we should avoid writing directly in the docs for the reasons I gave above - it should be possible to fiddle with automodapi to make this happen automatically in either of these two plans, though. (Maybe even in the scheme the current APE suggests, although we'd have to decide which of the two are the more "authoritative" - as-declared in code or as the comment in the template.) |
|
Regarding the point of duplicating information, I would vote that the template is the only definition (if possible). Just to check, this means the template gets installed in the package, since we can't rely on the user-modified configuration file, but we have to use the original template to define the possible configuration items, right? Another question is, how do we set the defaults since the template would have all values commented out? |
|
@eteq: With regard to context managers: Yes, I envision a base class for all of these context managers. Note that @eteq wrote:
At present, the config items are in seemingly random locations -- generally in the source file where the config option is used. That is not only hard for the user to guess, but ties its location to implementation and the organization of the source files, which may change. I'm proposing that the standard location for all config options is
Yes, the plan is to keep it, though obviously the "load everything and write a config file part" (IMHO the hackiest part of it) would go away.
I agree about the first two, but can't the docstring be automatically pulled into the documentation? That may mean
I assume, specifically, you mean that a configspec file exists in-source, from which is generated a template config file. The template config file will not have enough information (e.g. the type of the values) to properly generate a ConfigurationItem.
Don't we have that restriction now, given the use of
I should have been clearer on that -- I never meant that the documentation would be written directly in the .rst files, and had assumed automodapi would do the work here. My point there was that the config options should always be found (from the point of view of the built docs) in a standard place, just as we have our standard headings for other things already. |
|
@astrofrog: I think the way to do what Erik's suggesting (where the config file is the canonical and only definition of the config options) is to include configspec file in the source tree, and generate a template config file from that on-the-fly. configspec files handle data types and defaults for each of the options, but a config file would not. |
|
True enough - so yeah, at least some implementation might make some of the above discussion easier to foolow. |
|
Since there have been no objections to APE3 and no reports of issues with affiliated packages, I suggest that we go ahead and merge this and the associated pull request. Over the next month, before the 0.4.0 release, we might run into bugs, etc. but the sooner we merge this the sooner we will have this tested more widely. |
|
No objections. I don't have perms to merge this. I'll merge the implementation once Travis passes. |
There was a problem hiding this comment.
"there are number" -> "there are a number"
|
I gave it one more once-over and added a few more comments. Otherwise I'm 👍 |
|
Ok - in that case once @eteq agrees, we can merge this! |
|
The APE looks fine to me now! Some detailed items on astropy/astropy#2094 , but I suppose this can be accepted even if that's not finalized yet. |
|
@eteq - just to check, do we merge this then mark it as accepted in a separate commit? |
|
@astrofrog - what I've been doing is locally adding an accept commit and then manually merging into astropy-APEs/master, as that way I know for sure I haven't forgotten to commit the accept information. (And that auto-closes the PR, of course.) But what you're suggesting works fine too, as long as the accept commit actually appears, so whatever you find easier is fine with me. |
|
Shall we merge this now or wait until astropy/astropy#2094 is also ready? |
|
I think it's fine to merge this now as long as @mdboom doesn't foresee changes to this based on feedback in astropy/astropy#2094 |
|
Ok - @mdboom - can you confirm that this APE will not change based on the feedback in astropy/astropy#2094? |
|
No, it won't change. |
|
Ok, in that case, let's do this! Thanks @mdboom! |
APE3: New configuration system
|
👏 |
Ape7 update
docs: add verbiage about Coordinate
No description provided.