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

Create USAGE.rst #346

Merged
merged 4 commits into from
Sep 5, 2022
Merged

Conversation

victor-champonnois
Copy link
Member

@victor-champonnois victor-champonnois commented Jul 14, 2022

I know localization are needed for cooperator_website, is it also the case for cooperator ?

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2022

Codecov Report

Merging #346 (f0d71d1) into 12.0 (35443a5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             12.0     #346   +/-   ##
=======================================
  Coverage   56.58%   56.58%           
=======================================
  Files          97       97           
  Lines        3524     3524           
  Branches      560      560           
=======================================
  Hits         1994     1994           
  Misses       1451     1451           
  Partials       79       79           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -0,0 +1,8 @@
A localization module is needed with this module.

The following localization modules are available this repository:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following localization modules are available this repository:
The following localization modules are available in this repository:

@@ -0,0 +1,8 @@
A localization module is needed with this module.
Copy link
Member

Choose a reason for hiding this comment

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

What happens when none is installed ? Do we get a nice error ?

Copy link
Member Author

@victor-champonnois victor-champonnois Jul 26, 2022

Choose a reason for hiding this comment

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

No, there is no error.

Actually, this is part to the configuration of cooperator that is explained in the functional documentation. Since the documentation is linked in the readme, maybe this Usage file is not required. Or we could just make a usage file that points to the documentation ?

Copy link
Member

Choose a reason for hiding this comment

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

As discussed :

  • a link to the documentation is 👍
  • mentioning that localization is needed is nice
  • better solution is to make localization modules auto_installed

@victor-champonnois
Copy link
Member Author

todo (following discussion with @robinkeunen):

  • point to the functional doc in usage.rst
  • add auto-install in the l10n_xx_cooperator modules

@victor-champonnois
Copy link
Member Author

todo (following discussion with @robinkeunen):

* point to the functional doc in usage.rst

* add auto-install in the l10n_xx_cooperator modules

Done

@victor-champonnois victor-champonnois requested review from enricostano and robinkeunen and removed request for enricostano September 1, 2022 12:13
@@ -35,4 +35,5 @@
],
"installable": True,
"application": False,
"auto-install": True,
Copy link
Member

Choose a reason for hiding this comment

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

Is cooperator_website needed in the depends here ? If so, the localization will only be installed if all 4 modules are installed.

Besides the auto_install feature, it also means we can't install the localization without installing cooperator_website.

Same story for other localizations.

Copy link
Member

Choose a reason for hiding this comment

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

En passant, l10n_be_invoice_bba is empty in v14 so we can skip that dependency there.

Copy link
Member Author

@victor-champonnois victor-champonnois Sep 2, 2022

Choose a reason for hiding this comment

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

Is cooperator_website needed in the depends here ? If so, the localization will only be installed if all 4 modules are installed.

Besides the auto_install feature, it also means we can't install the localization without installing cooperator_website.

Same story for other localizations.

Yes it is needed because the module overload the subscription form template. I don't see a way around this now, maybe cooperator_website should also be auto-install ?

Copy link
Member Author

Choose a reason for hiding this comment

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

En passant, l10n_be_invoice_bba is empty in v14 so we can skip that dependency there.

Noted

Copy link
Member

Choose a reason for hiding this comment

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

Ok then I think you should add in known_caveats of ROADMAP.rst that this module will have to be split if someone needs to install one module without the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@victor-champonnois
Copy link
Member Author

PR forward ported here : #368

@robinkeunen robinkeunen requested review from robinkeunen and removed request for enricostano September 5, 2022 15:08
@robinkeunen
Copy link
Member

/ocabot merge minor

@github-grap-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-346-by-robinkeunen-bump-minor, awaiting test results.

github-grap-bot added a commit that referenced this pull request Sep 5, 2022
Signed-off-by robinkeunen
@github-grap-bot
Copy link
Contributor

It looks like something changed on 12.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 12.0-ocabot-merge-pr-346-by-robinkeunen-bump-minor, awaiting test results.

github-grap-bot added a commit that referenced this pull request Sep 5, 2022
Signed-off-by robinkeunen
@github-grap-bot
Copy link
Contributor

It looks like something changed on 12.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 12.0-ocabot-merge-pr-346-by-robinkeunen-bump-minor, awaiting test results.

github-grap-bot added a commit that referenced this pull request Sep 5, 2022
Signed-off-by robinkeunen
@github-grap-bot
Copy link
Contributor

@robinkeunen The merge process could not be finalized, because command git push origin 12.0-ocabot-merge-pr-346-by-robinkeunen-bump-minor:12.0 failed with output:

To https://github.com/coopiteasy/vertical-cooperative
 ! [rejected]          12.0-ocabot-merge-pr-346-by-robinkeunen-bump-minor -> 12.0 (non-fast-forward)
error: failed to push some refs to 'https://***@github.com/coopiteasy/vertical-cooperative'
hint: Updates were rejected because a pushed branch tip is behind its remote
hint: counterpart. Check out this branch and integrate the remote changes
hint: (e.g. 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

@robinkeunen robinkeunen force-pushed the 12.0-add-usage-localization-module branch from 5b63655 to 425d9a5 Compare September 5, 2022 15:41
@robinkeunen
Copy link
Member

/ocabot merge minor

@github-grap-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-346-by-robinkeunen-bump-minor, awaiting test results.

@github-grap-bot github-grap-bot merged commit 5988d56 into 12.0 Sep 5, 2022
@github-grap-bot github-grap-bot deleted the 12.0-add-usage-localization-module branch September 5, 2022 15:47
@github-grap-bot
Copy link
Contributor

Congratulations, your PR was merged at 71ca25b. Thanks a lot for contributing to coopiteasy. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants