Skip to content

Improve readme and fix dep#733

Merged
legalsylvain merged 2 commits into
OCA:9.0from
akretion:keychain-improve_readme_and_dep
Feb 16, 2017
Merged

Improve readme and fix dep#733
legalsylvain merged 2 commits into
OCA:9.0from
akretion:keychain-improve_readme_and_dep

Conversation

@hparfr

@hparfr hparfr commented Feb 13, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

👍

@bealdav

bealdav commented Feb 16, 2017

Copy link
Copy Markdown
Member

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

Remove explicit dependency to base.
@hparfr

hparfr commented Feb 16, 2017

Copy link
Copy Markdown
Contributor Author

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

Fixed.

Thanks for your reviews

@bealdav

bealdav commented Feb 16, 2017

Copy link
Copy Markdown
Member

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

@hparfr

hparfr commented Feb 16, 2017

Copy link
Copy Markdown
Contributor Author

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

@tafaRU tafaRU left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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