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

Allow a directory as config install source #53

Merged
merged 7 commits into from
Aug 23, 2019

Conversation

Lawrencemm
Copy link

@Lawrencemm Lawrencemm commented Aug 22, 2019

Closes #23.

@Lawrencemm Lawrencemm changed the title Allow a directory as config install location. Allow a directory as config install source. Aug 22, 2019
@Lawrencemm Lawrencemm changed the title Allow a directory as config install source. Allow a directory as config install source Aug 22, 2019
Copy link

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

A couple of details, but I really like it. Thanks!

src/main/java/conan/ui/configuration/ConanConfig.java Outdated Show resolved Hide resolved
});
configInstallLocation.getEmptyText().setText("Directory or URL");
Copy link

Choose a reason for hiding this comment

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

Suggested change
configInstallLocation.getEmptyText().setText("Directory or URL");
configInstallLocation.getEmptyText().setText("Git repository, local folder or ZIP file (local or http)");

Maybe it is too long, but it is what we currently have in the help message

Copy link
Author

Choose a reason for hiding this comment

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

Looking here: https://docs.conan.io/en/latest/reference/commands/consumer/config.html

We might want to provide a checkbox to add --type git to the config install command.

You can also force the git download by using --type git (in case it is not deduced from the URL automatically):

Copy link
Author

Choose a reason for hiding this comment

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

The above requires the checkbox to be persisted.

I'm not currently comfotable making changes to the persistency because we don't seem to have any unit tests for it.

I might make an issue for this force_git thing, and come back to it after we have some more robust testing. What do you think?

Copy link

Choose a reason for hiding this comment

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

Yes, don't worry about that. IMHO all the config should be refactored in the future, t could be better to have a submenu for the configuration populated by [all] the values in .conan/conan.con and .conan/remotes.json and let the user modify them individually (these values would be persisted by Conan itself, not by CLion)... and the download/install button would be just an action, and at that moment it will request a URL/file.

It is a big change, I'm not sure about it, but it would make the integration much better if the user doesn't need to go to the command line to do some modifications.

Copy link
Author

Choose a reason for hiding this comment

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

I like the sound of that. 👍

Copy link

Choose a reason for hiding this comment

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

The only problem is Conan changing the format of those files. It is easier to parse/write the files than to use json output from CLI interface. We would need to abstract file format and implement reader/writers for different Conan versions.

@jgsogo jgsogo added this to the 1.2.0 milestone Aug 22, 2019
@Lawrencemm
Copy link
Author

I noticed that I had a bunch of wrong wordings/inconsistencies in the UI because some things are hard coded in the ConanConfig.form file.

It seems neither of us picked up on this so it might be worth making a policy that PRs which affect the UI must provide some screenshots or something, otherwise such mistakes might make it through and be annoying later.

Anyway here are some screens as of commit 42ec914:

image

Failure to install:

image

Successfully installed:

image

Note that it says nothing on success. It might be better to give feedback if config install is successful.

@Lawrencemm
Copy link
Author

One more thing: there should probably be a file picker widget here for locating local files/directories. Let me know if you'd like that included or left to a future enhancement.

@jgsogo
Copy link

jgsogo commented Aug 23, 2019

Thanks for the screenshots 🙌

I would like to know your opinion about #53 (comment) before thinking about adding other widgets to this UI form (which, by the way, I would leave for the future).

Copy link

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

LGTM 😺

@Lawrencemm
Copy link
Author

👍 I'm happy

@jgsogo jgsogo added the enhancement New feature or request label Aug 23, 2019
@jgsogo jgsogo merged commit 51988d0 into conan-io:dev Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow local directories not just URLs as the source for installing configs
2 participants