-
Notifications
You must be signed in to change notification settings - Fork 104
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
Website configuration #52
base: master
Are you sure you want to change the base?
Website configuration #52
Conversation
Could you fix the length of the commit messages? The first line of a git commit message is used as a brief summary, and should typically be kept under 70 characters in order to avoid truncation when viewing commit logs. |
I did a git rebase -i and reworded then a force push. Not sure what's going on as looking at the pull request elliot42s previous commit has got sucked in as a change. I checked my console output and the rebase claims it only touched my commits so hell knows what's going on. Not sure if this is a github bug or forcing was a stupid idea. If that causes a problem with the merge let me know and I'll kill the branch and start a new fork. |
Ignore previous. Looks like a github consistency bug. Force refresh on the browser and all history looks correct. |
Thanks. I think that perhaps we could shorten "configuration" to "config" without losing any clarity, i.e. |
:errorDocument - the value for the error document" | ||
[cred name configuration] | ||
(let [routing-rules-beans (map (partial java/to-java RoutingRule) (:routingRules configuration)) | ||
configuration-bean (java/to-java BucketWebsiteConfiguration (assoc configuration :routingRules routing-rules-beans))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you ensure the lines are not too long? i.e. within 80 or 90 characters.
Oh, also, I note that the keys for the new functions are camel-cased, rather than hyphenated. Could you change the keys to use the hyphenated form, e.g. |
Updated:
General question: I'm not sure about you but the amount of effort for mapping between beans seems really high especially considering it's all just boiler plate. Unfortunately data.java doesn't support hyphenation and the activity on the project is low so I'm not sure how well looked after it is. Would consider forking and patching it (given time etc.). Do you know of any alternatives. |
Added functions to manage a bucket's website configuration.
I used clojure.java.data to convert to and from the BucketWebsiteConfiguration bean.
There is some complexity as the lib doesn't handle typed collections very well due to type erasure so the vector of routing rules have to be converted explicitly to a vector of RoutingRule beans as a first step. However, due to the complex nature of the BucketWebsiteConfiguration bean and RoutingRule bean I felt the clojure.java.data library was still preferable. You may of course disagree :)