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

Fix #393: 'dub describe' now adds an 'active' field to each dependency #577

Closed
wants to merge 1 commit into from

Conversation

atilaneves
Copy link
Contributor

I reckon the codebase still has some refactoring to do; I tried to keep the patch as simple as possible to implement the desired behaviour. I tested the patch by actually using the output of dub describe and it seems to work like a treat.

@atilaneves
Copy link
Contributor Author

Travis says the build fails but dub test works fine. Seems to be an environment variable issue. I think this patch correctly implements the enhancement request.

@Geod24
Copy link
Member

Geod24 commented Jun 10, 2015

If you look at the console output, you'll see it fails in the (limited) test-suite:

https://travis-ci.org/D-Programming-Language/dub/jobs/65049031

@atilaneves
Copy link
Contributor Author

Yep, now I also know how to run the test suite. Sorry for that. I'll update the PR once I've fixed the issue.

@s-ludwig
Copy link
Member

We should separate the source file active state from the package state. The latter is fine as it is here, but the former belongs to a more general solution for a build-tool related description. I've started this in the form of a separate "targets" field in a separate branch.

@s-ludwig
Copy link
Member

BTW, regarding source file active state there is already "role": "source" vs. "role": "unusedSource" that should to the same thing as the "active" field, unless I'm missing something.

I've pulled the other "dub describe" improvements now. If you are lacking the time, I can have a look at adding the package "active" state to the new code.

@atilaneves
Copy link
Contributor Author

I went with active because it was mentioned in the issue, and also because I was sure it would be a non-breaking change. I'm not sure about having time, I'll try and take a look at this soon.

@s-ludwig
Copy link
Member

"active" is actually fine for packages! Just for source files it's already there in the form of the "role" field. Since I'd like to get this into the upcoming release (to be released ASAP), let me just take it over from here and adjust it for the latest changes on master. I'll try to post a final beta for 0.9.24 later today.

@s-ludwig s-ludwig closed this in fa205b3 Jun 30, 2015
@s-ludwig
Copy link
Member

Okay, done now. Just one note about "active" vs. "targets" - the whole "packages" field, including the "active" field for each package, is more suited for consumption by IDEs (e.g. to visualize if a certain package is built or not). For build tools, the "targets" field is the right choice. It correctly handles all kinds of semantic details, such as inheritance of compiler flags or application of build modes and target types, which is not true for the former.

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.

None yet

3 participants