Skip to content

Conversation

hparfr
Copy link
Contributor

@hparfr hparfr commented Feb 13, 2017

1° Readme: (in example) prefix function with _
2° Dep: add a depency to base_setup in order to use xml id "base.menu_config" in https://github.com/OCA/server-tools/blob/9.0/keychain/views/keychain_view.xml#L47

Readme: (in example) prefix function with _
Dep: add a depency to base_setup in order to use xml id "base.menu_config"
@florian-dacosta
Copy link
Contributor

👍

@bealdav
Copy link
Member

bealdav commented Feb 16, 2017

Please remove base, if you add base_setup. Otherwise, 👍
"Qui peut le + peut le moins"

Remove explicit dependency to base.
@hparfr
Copy link
Contributor Author

hparfr commented Feb 16, 2017

Please remove base, if you add base_setup. Otherwise, 👍

Fixed.

Thanks for your reviews

@bealdav
Copy link
Member

bealdav commented Feb 16, 2017

Why several commits here, for 2 lines ? --force

@hparfr
Copy link
Contributor Author

hparfr commented Feb 16, 2017

--forcing a push on a public branch is not a good thing
It breaks the history of the comments (like inline review).

Copy link
Member

@tafaRU tafaRU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM even though I'd prefer commits to be squashed to only one.

@legalsylvain
Copy link
Contributor

thanks.

@legalsylvain legalsylvain merged commit cab186d into OCA:9.0 Feb 16, 2017
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (13.0)
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.

5 participants