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

Add namespaces to generate-themes-doc.fish #154

Merged
merged 1 commit into from
Nov 3, 2015

Conversation

Pyppe
Copy link
Contributor

@Pyppe Pyppe commented Nov 3, 2015

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

That's cool!

Do we need the THEMES-NAMESPACE? If not, I'd like to remove it to it's less verbose.

@Pyppe
Copy link
Contributor Author

Pyppe commented Nov 3, 2015

Well.. we don't, but personally I'd prefer it, because it stands out and makes it more explicit that something has been replaced/generated there. At least I think we should have some prefix other than the theme name itself (but of course it could be shorter).

Off-topic speculation...

Also... there's starting to be too much of regex-magic going on in generate-themes-doc.fish for my taste. It's not very safe any more. For example, it doesn't take into account that there might be reference-style link syntax inside code blocks. The current implementation would replace those too. But to make it more robust, I think a real programming language might be in order. At least my fish scripting skills are starting to be too limited to implement that kind of behavior.

@derekstavis
Copy link
Member

I'd prefer trying something like asciicast for theme previews. Btw, I don't know what level of automation it allows.

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

There has been some talks around it, we might want to only use a theme screenshot instead of the full readme. What's your opinion on that?

@Pyppe
Copy link
Contributor Author

Pyppe commented Nov 3, 2015

Good idea having the screenshots only. That is actually what I tried to do at first (couple of months ago when we added the generation script). However, IIRC it was very difficult to determine from the README file what actually is the screenshot. There are so many different ways people have added those. That's why I actually included the whole README-file instead of trying to parse only the screenshot.

@derekstavis
Copy link
Member

Maybe we could create a script which is executed inside asciinema recorder. A script would ensure all themes get equal preview treatment. Then with the preview uploaded to asciinema we could use its URLs.

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

We could add a placeholder on the template for a screenshot and only use the screenshot if it's on that location.

@derekstavis what I don't like is that it doesn't allow people to show off a specific customisation of your theme.

@Pyppe
Copy link
Contributor Author

Pyppe commented Nov 3, 2015

@bpinto Yeah, placeholder would solve the issue. Alternative approach would be to use e.g. JavaScript (node.js) to replace the generation script. Then we could e.g. parse the READMEs more robustly, and find screenshots better (comparing e.g. the image sizes).

^ But that would be quite an additional dependency for such a trivial task as listing theme screenshots.

My humble suggestion would be to merge this (because it's better than the current master), and create a new issue for solving the issue better. Perhaps by requiring the screenshot to be declared in certain way, as @bpinto suggested.

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

I agree, could you rebase? We just merged another PR. 👯

@derekstavis do you agree?

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

By the way, I don't want to limit to screenshots only, could be whatever, as long as it's in the placeholder.

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

Fixes: #116

@Pyppe
Copy link
Contributor Author

Pyppe commented Nov 3, 2015

I don't think we need to rebase? There's nothing conflicting, and it's just one commit.

@Pyppe
Copy link
Contributor Author

Pyppe commented Nov 3, 2015

But rebase I did, nevertheless. 😁

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

We always rebase before merging to keep the git history clean. It's a philosophy of life, I guess. 😂

Thanks!

@bobthecow
Copy link
Member

I'm with @bpinto. I'd considered doing an asciicast script, but a generic one wouldn't work very well. For example, the bobthefish screenshot specifically goes into a second-level-from-root directory which isn't owned by the current user to show off permissions color, truncated paths, and non-repo directory styles. It also changes user, and sshes into another box to show off things with users and hostnames. It's likely the only theme with support for that exact combination of features. If any other theme used the same sequence, there would be a bunch of boring steps where nothing really changed.

What if, instead, we added a started script to the theme template, and let people customize and record their own?

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

That's an interesting approach @bobthecow

@bpinto
Copy link
Member

bpinto commented Nov 3, 2015

Merging it. Will someone open an issue so we can discuss how to improve it?

Thanks for the fast PR @Pyppe

bpinto added a commit that referenced this pull request Nov 3, 2015
Add namespaces to generate-themes-doc.fish
@bpinto bpinto merged commit b3fc4c9 into oh-my-fish:master Nov 3, 2015
@bobthecow
Copy link
Member

I'd say 👍 on this for now. It fixes the immediate problem, even if it's a bit more brittle than we'd like. In the event that it breaks, someone should notice and we can address it then :)

For the future, though, I'd be interested in a more "summary" style approach. Maybe parse the theme README looking for <!-- OMF-SUMMARY --> ... <!-- END-OMF-SUMMARY --> and generating a generic one if it doesn't exist?

@Pyppe Pyppe deleted the theme-doc-namespace branch November 3, 2015 18:03
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

4 participants