-
Notifications
You must be signed in to change notification settings - Fork 6
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
Api update #32
base: main
Are you sure you want to change the base?
Api update #32
Conversation
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming .rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming .rubocop.yml: Style/AsciiIdentifiers has the wrong namespace - should be Naming Error: The `Lint/InvalidCharacterLiteral` cop has been removed since it was never being actually triggered. (obsolete configuration found in .rubocop.yml, please update it) The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming .rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming .rubocop.yml: Style/AsciiIdentifiers has the wrong namespace - should be Naming Error: The `Lint/InvalidCharacterLiteral` cop has been removed since it was never being actually triggered. (obsolete configuration found in .rubocop.yml, please update it) The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Na...
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming .rubocop.yml: Style/AsciiIdentifiers has the wrong namespace - should be Naming .rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming Error: The `Lint/InvalidCharacterLiteral` cop has been removed since it was never being actually triggered. (obsolete configuration found in .rubocop.yml, please update it) The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.
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.
Dear @soenkex thank you for your pull request and I am sorry that we didn't respond for a while. I added some comments about code styling issues and questions about the intend of some of your changes.
More importantly you didn't include any tests with your pull request. To be merged, the pull request needs to test every publicly exposed method.
Please also refactor your commits. Each commit should only change/add a single "feature", e.g. a single resource like the Plan
resource. Each commit should also contain tests for the changes and a line in the changelog. You can use git rebase -i origin/master
to refactor your commits.
Please take a look at some of the older commits like 969fc42 to get a glance at how commits should look like.
def camelize(term) | ||
string = term.to_s | ||
string = string.sub(/^[a-z\d]*/) { $&.capitalize } | ||
string.gsub(/(?:_|(\/))([a-z\d]*)/) { "#{$1}#{$2.capitalize}" }.gsub('/', '::') | ||
if (string.downcase == '_c') |
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.
This seems to me like some really special behaviour. Could you please elaborate why this is necessary and give some examples?
@@ -75,6 +75,7 @@ def request(method, path, params = {}) | |||
def connection_options | |||
@connection_options ||= { | |||
:builder => middleware, | |||
:ssl => {verify_mode: OpenSSL::SSL::VERIFY_NONE}, |
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.
You stated that you had issues with SSL. I think it would be better to fix your issues or solve them otherwise. Changing the SSL options in the gem is somewhat risky and untransparent to the user of the gem.
options = options.camelize_keys | ||
patch "api/v1/components/#{component_id}", options | ||
end | ||
|
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.
Please do not add empty-lines between end
s. This should be applied to all files of this pull request.
|
||
end | ||
end | ||
end |
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.
Please add a newline at the end of the file, your editor should have an option to do this automatically for you. This should be applied to all files of this pull request.
lib/pactas_itero/api/contracts.rb
Outdated
def subscribe_component_for_contract(contract_id, options = {}) | ||
options = options.camelize_keys | ||
post "api/v1/contracts/#{contract_id}/componentsubscriptions", options | ||
|
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.
Please do not add empty lines before end
s.
…_update Conflicts: lib/pactas_itero/api/contracts.rb
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Na...
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming .rubocop.yml: Style/AsciiIdentifiers has the wrong namespace - should be Naming .rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming Error: The `Lint/InvalidCharacterLiteral` cop has been removed since it was never being actually triggered. (obsolete configuration found in .rubocop.yml, please update it) The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.
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.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Na...
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming .rubocop.yml: Style/AsciiIdentifiers has the wrong namespace - should be Naming .rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming .rubocop.yml: Lint/ConditionPosition has the wrong namespace - should be Layout Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it) The `Lint/InvalidCharacterLiteral` cop has been removed since it was never being actually triggered. The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`. obsolete parameter MaxLineLength (for Style/IfUnlessModifier) found in .rubocop.yml `Style/IfUnlessModifier: MaxLineLength` has been removed. Use `Metrics/LineLength: Max` instead
Added a few new API-Resources and some README updates
I have set SSL-verification to VERIFY_NONE in client.rb line 78 - as SSL-Verification drives me crazy while testing. Change this line to whatever is needed...
Thanks!