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

[rfc] Change 'conan search --table' output #6832

Merged
merged 14 commits into from Apr 28, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Apr 11, 2020

Changelog: Feature: Change HTML output for conan search --table command.
Docs: conan-io/docs#1676

Close #6842

TODO: There are other open issues requesting a better search for binaries, I think this feature will close them, need to gather them.

  • Tests needed

  • (¿?) Improve look and feel: add header, footer, links,... to the HTML

  • Proper path for conans/assets/templates/...

  • Is the list of requires retrieved from the servers? #6859

    • requires from conan_server are retrieved under key full_requires
    • requires from https://conan.bintray.com are retrieved under key requires

Here I'm proposing a completely new HTML for the conan search --table command. From a matrix it is converted into a table using the awesome Datatables javascript library.

Probably it helps also with all the "missing binaries for...." related issues.

image

This PR introduces a templates pattern into Conan I would like to talk about too (removed in
795d864)

def get_template(template_name, templates_folder):
# Loader order: first current working dir, then 'templates' folder, fallback to dictionary
loader = ChoiceLoader([
FileSystemLoader(os.getcwd()),
Copy link
Member Author

@jgsogo jgsogo Apr 11, 2020

Choose a reason for hiding this comment

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

I would love to move the env object into the app.cache one, so it is created only once and it can take advantage of internal cache. Not really important right now, the Conan client is running only one command each time it is loaded... but from the conan_api POV it would be important.

Copy link
Member Author

@jgsogo jgsogo Apr 11, 2020

Choose a reason for hiding this comment

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

If we agree on moving this to the app.cache I'll make a PR with that refactor.

Copy link
Member

@memsharded memsharded Apr 11, 2020

Choose a reason for hiding this comment

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

Maybe I would wait for this functionality of different templates. I would focus first on the replacement of the current table, and will wait for this, as I understand this will be used also for other templates, like the graph one.

@@ -0,0 +1,96 @@

Copy link
Member Author

@jgsogo jgsogo Apr 11, 2020

Choose a reason for hiding this comment

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

This should be an actual .html file installed together with Conan into the library folder.

Copy link
Member

@memsharded memsharded Apr 11, 2020

Choose a reason for hiding this comment

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

The main problem with that is how error prone is to create the installers with pyinstaller to embed and use other resources than code. It is tricky, and installers aren't that tested after the creation, so it is easy to have broken installers.

Copy link
Member

@memsharded memsharded left a comment

Looking good, keep going.

conans/assets/templates/search_table_html.py Show resolved Hide resolved
@@ -0,0 +1,96 @@

Copy link
Member

@memsharded memsharded Apr 11, 2020

Choose a reason for hiding this comment

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

The main problem with that is how error prone is to create the installers with pyinstaller to embed and use other resources than code. It is tricky, and installers aren't that tested after the creation, so it is easy to have broken installers.


import conans.assets.templates.search_table_html

SEARCH_TABLE_HTML = 'output/search_table.html'
Copy link
Member

@memsharded memsharded Apr 11, 2020

Choose a reason for hiding this comment

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

The "output" a bit unnecessary? Every html will be an output?

I would like to see "binaries" in the name, SEARCH_BINARIES_TABLE or something like that, to differentiate of other possible searches.

Copy link
Member Author

@jgsogo jgsogo Apr 11, 2020

Choose a reason for hiding this comment

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

Something to talk about. I see generators using this pattern too... and sometimes the CLI as well 😅 This is the reason why I prefer to have a folder, maybe output/search_table.html is not the best name, we have time to think a better one (I would like it to be coupled to the command that uses it, now it is search --table=xxxx.html, but I agree to align it with the future of the CLI)

def get_template(template_name, templates_folder):
# Loader order: first current working dir, then 'templates' folder, fallback to dictionary
loader = ChoiceLoader([
FileSystemLoader(os.getcwd()),
Copy link
Member

@memsharded memsharded Apr 11, 2020

Choose a reason for hiding this comment

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

Maybe I would wait for this functionality of different templates. I would focus first on the replacement of the current table, and will wait for this, as I understand this will be used also for other templates, like the graph one.

@jgsogo jgsogo added this to the 1.25 milestone Apr 17, 2020
@jgsogo jgsogo marked this pull request as ready for review Apr 17, 2020
@jgsogo jgsogo requested a review from czoido Apr 17, 2020
Copy link
Member

@memsharded memsharded left a comment

Good job.

I think I will slightly change some of the html functionality:

  • Instead of "Search xxxx" I would use "Filter xxx"
  • I would move the "Search xxxx" boxes to the top row, not the bottom. Probably the most useful feature, and hard to find if Select >10 occurrences

The table could benefit from some styling (like narrower columns when possible to fit), but that can wait, no need to change it now.

@czoido
Copy link
Contributor

@czoido czoido commented Apr 22, 2020

I think this is very nice and an improvement over the current table. I have some minor personal suggestions, check them in case you agree with any of those:

  • Show a greater number of packages or even all packages by default.
  • Seems like the default order is by the package ID, I would prefer to see them ordered by the os or compiler or any other more meaningful field.
  • Using a monospaced font.
  • Now the number of generated packages is shown on the left lower corner as part of the table, I would make it part of the title or a place where it was more visible.

@memsharded
Copy link
Member

@memsharded memsharded commented Apr 22, 2020

I agree especially with the first one, show all packages by default, but then, the "Search" boxes will be hidden at the bottom, so I would do these changes at the same time.

In any case, the core Conan functionality looks correct, IMO if it is only improvements over the html, and we are tight on time for this release, this can be released and wait for another iteration for these style changes.

czoido
czoido approved these changes Apr 22, 2020
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Apr 22, 2020

I really prefer to keep the output as close to the canonical example provided by Datatables as possible and not to spend time doing CSS/JS the wrong way. If people want a different output, they can override the template (or send a PR to improve this one, if it doesn't make it more complex: it should serve as an example).

Changes:

  • monospaced font for the package_id column. I think it is the only one that could benefit from that.
  • change "Search" to "Filter"

Other changes may require to tweak the Datatables standard example too much (or I'm not understanding them, please, change the HTML locally and send a screenshot/snippet). About adding more rows, what is the advantage? I really prefer the table to fit on the screen than to scroll down to see the footer.

Anyway this HTML should be an example, functional, but as simple as possible. Then we should let the user override the template to add all the customization that they need.

@memsharded
Copy link
Member

@memsharded memsharded commented Apr 22, 2020

Anyway this HTML should be an example, functional, but as simple as possible. Then we should let the user override the template to add all the customization that they need.

Well, I think there is still some improvements that will benefit the majority of users. Take into account that only very few users will be modifying the html template for their needs, so our default one might be a bit better yet. Just we do not need to do these improvements now. I think that it is likely that we might get improvements and PRs from the users too for the default template.

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Apr 22, 2020

Anyway this HTML should be an example, functional, but as simple as possible. Then we should let the user override the template to add all the customization that they need.

Well, I think there is still some improvements that will benefit the majority of users. Take into account that only very few users will be modifying the html template for their needs, so our default one might be a bit better yet. Just we do not need to do these improvements now. I think that it is likely that we might get improvements and PRs from the users too for the default template.

If we go ahead with the templates, with a simple conan config install https://github.com/conan-io/templates -sf fancy-output -tf templates the user can get a fancy template from a community-contributed gallery. 😺

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Apr 27, 2020

Done. Added docs, updated the image of the table... 🚀

@czoido czoido merged commit 2c11c49 into conan-io:develop Apr 28, 2020
2 checks passed
@jgsogo jgsogo deleted the feat/search-template branch Apr 28, 2020
ApatheticsAnonymous pushed a commit to ApatheticsAnonymous/conan that referenced this issue Oct 6, 2020
…f select_autoescape which was introduced in Jinja2 v2.9
ApatheticsAnonymous added a commit to ApatheticsAnonymous/conan that referenced this issue Oct 6, 2020
…f select_autoescape which was introduced in Jinja2 v2.9
memsharded pushed a commit that referenced this issue Oct 6, 2020
…_autoescape which was introduced in Jinja2 v2.9 (#7823)
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.

3 participants