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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] serve: Add '--simple-index' parameter #265

Merged
merged 6 commits into from
Nov 7, 2019
Merged

[FEATURE] serve: Add '--simple-index' parameter #265

merged 6 commits into from
Nov 7, 2019

Conversation

jkoenig134
Copy link
Contributor

Showing hidden files and a detailed view is now possible via command line options.
Needs SAP/ui5-server#255 to work properly.

Pull Request Checklist

@jkoenig134
[FEATURE] add options to serve.js
7b84398 
add options show-hidden and show-details to the serve module
@jkoenig134
[FIX] add showHidden and showDetails to tests
e7c9628
@CLAassistant
Copy link

CLAassistant commented Nov 4, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Nov 4, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 89.375% when pulling 30b7ff5 on jkoenig134:master into 715253b on SAP:master.

RandomByte
RandomByte previously requested changes Nov 4, 2019
View reviewed changes
lib/cli/commands/serve.js Outdated Show resolved Hide resolved
lib/cli/commands/serve.js Outdated Show resolved Hide resolved
lib/cli/commands/serve.js Show resolved Hide resolved
lib/cli/commands/serve.js Outdated Show resolved Hide resolved
jkoenig134 and others added 2 commits November 4, 2019 13:46
@jkoenig134 @RandomByte
[FIX] update description for show-hidden option
24a3263 
Co-Authored-By: Merlin Beutlberger <r@ndombyte.net>
@jkoenig134
[INTERNAL] better descriptions, change defaults
e2a691b 
descriptions changed
show-hidden and show-details defaults now to true
@jkoenig134
Copy link
Contributor Author

@RandomByte i changed the defaults and descriptions

@jkoenig134
Copy link
Contributor Author

When is the next release? And will my changes go online then?

@jkoenig134 jkoenig134 requested a review from RandomByte November 4, 2019 14:47
@RandomByte
Copy link
Member

Thanks! I'll merge the server change right away and this one probably tomorrow. I want to go over the naming of the CLI parameters with some colleagues. We'll probably also do a release then 👍

@jkoenig134
Copy link
Contributor Author

@RandomByte thanks a lot. My idea about the naming is not perfect, i know.
Maybe hide-dotfiles instead of show-hidden would fit better in the tooling?

@RandomByte
Copy link
Member

Yeah, I also found hide-dotfiles being used by the lighttpd project: https://redmine.lighttpd.net/projects/lighttpd/wiki/Docs_ModDirlisting

@jkoenig134
Copy link
Contributor Author

Feel free to change it, maintainer edit is active!

@RandomByte
Copy link
Member

CC @matz3 @svbender

@RandomByte
Copy link
Member

@jkoenig134 Sorry but due to absence of some colleagues it might take another day or two to get this merged and released

@jkoenig134
Copy link
Contributor Author

No problem!

@matz3
Copy link
Member

matz3 commented Nov 6, 2019

I don't really mind about the naming.
I rather wonder whether this is really required as a CLI option. For the UI5 use cases I would expect that dotfiles should always be shown, as we have several of them (.library, .theming, .theme, ...).

@jkoenig134 jkoenig134 changed the title [FEATURE] implement showHidden and showDefault [FEATURE] implement showHidden and showDetails Nov 6, 2019
@RandomByte
Copy link
Member

hey @jkoenig134, we discussed this again internally. As Matthias already wrote, UI5 comes with various dot files, so we wonder whether it actually makes sense to offer a switch to hide them.

Also, we wonder whether there is any demand in the non-detail directory listing.

Do you think you will use these parameters yourself or know of a use case where they would be helpful?

@jkoenig134
Copy link
Contributor Author

Hey @RandomByte ! I totally see that point. The dotfile parameter is not necessary.
The non-detail parameter is. We are migrating from a grunt-Tooling where the non detail view was the default. And I think it also has a really good handling.

@RandomByte
Copy link
Member

Fair enough. It pretty much boiled down to not having the dotfile parameter and renaming the one to activate the simplified "tile" view to --simple-index. I'll make the necessary changes now.

@jkoenig134
Copy link
Contributor Author

Thanks. Sounds great.

@RandomByte
[INTERNAL] Remove showHidden- and rename showDetails parameters
30b7ff5 
Rename parameter "showDetails" to "simpleIndex" with negated meaning

Depends on SAP/ui5-server#256
@RandomByte RandomByte changed the title [FEATURE] implement showHidden and showDetails [FEATURE] serve: Add '--simple-index' parameter Nov 7, 2019
RandomByte added a commit to SAP/ui5-tooling that referenced this pull request Nov 7, 2019
@RandomByte
[INTERNAL] Document new 'serve --simple-index' CLI parameter
e0def3e 
As implemented in SAP/ui5-cli#265
@RandomByte RandomByte mentioned this pull request Nov 7, 2019
Merged
@RandomByte RandomByte requested a review from matz3 November 7, 2019 13:11
@RandomByte RandomByte merged commit 1ae4922 into SAP:master Nov 7, 2019
@RandomByte
Copy link
Member

RandomByte commented Nov 7, 2019 •
edited
Loading

Actually merged as dbe195e, GitHub UI struggled a bit with me composing the squash commit while the master got new commits 🤷🏼‍♂️

RandomByte pushed a commit that referenced this pull request Nov 7, 2019
@jkoenig134 @RandomByte
[FEATURE] serve: Add '--simple-index' parameter (#265)
dbe195e 
The directory listing of the UI5 Server has been overhauled by
integrating parts of the serve-index[1] project (see [2]).

The '--simple-index' flag allows to toggle between a detailed- and a
simplified rendering of the directory index.

Thanks to @jkoenig134 for this contribution!

[1] https://github.com/expressjs/serve-index
[2] SAP/ui5-server#253
RandomByte added a commit to SAP/ui5-tooling that referenced this pull request Nov 7, 2019
@RandomByte
[INTERNAL] Document new 'serve --simple-index' CLI parameter
bb53c5a 
As implemented in SAP/ui5-cli#265
@jkoenig134
Copy link
Contributor Author

jkoenig134 commented Nov 7, 2019 •
edited
Loading

Finally! 😆 Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matz3 matz3 matz3 approved these changes

@RandomByte RandomByte Awaiting requested review from RandomByte

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jkoenig134 @CLAassistant @coveralls @RandomByte @matz3