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

Website configuration #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

petergillardmoss
Copy link

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 :)

@weavejester
Copy link
Owner

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.

@petergillardmoss
Copy link
Author

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.

@petergillardmoss
Copy link
Author

Ignore previous. Looks like a github consistency bug. Force refresh on the browser and all history looks correct.

@weavejester
Copy link
Owner

Thanks. I think that perhaps we could shorten "configuration" to "config" without losing any clarity, i.e. get-bucket-website-config and update-bucket-website-config.

: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))]
Copy link
Owner

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.

@weavejester
Copy link
Owner

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. :routing-rules rather than :routingRules. Idiomatic Clojure code prefers the former over the latter.

@petergillardmoss
Copy link
Author

Updated:

  • use hyphenated keys
  • replace configuration with config
  • kept line lengths down

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.

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.

2 participants