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

[IMP] beesdoo_shift: add name_get on task_template #302

Open
wants to merge 1 commit into
base: 12.0
Choose a base branch
from

Conversation

tfrancoi
Copy link
Contributor

this allow to use generated task template name
and not add all info of the template directly in the name

@tfrancoi
Copy link
Contributor Author

TODO: retarget on 12.0 once add-polln-shift is merged
Move this to beesdoo_shift

Base automatically changed from 12.0-add-polln-shift to 12.0 February 24, 2022 17:32
@tfrancoi tfrancoi force-pushed the 12.0-task_template-name_get branch from d18b0bd to 01974fb Compare February 24, 2022 17:47
Copy link
Member

@robinkeunen robinkeunen left a comment

Choose a reason for hiding this comment

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

TODO:

  • will need a migration script.
  • move to beesdoo_shift

time_domain = []
FIELDS = ['planning_id', 'name', 'day_nb_id']
for n in name.split(" "):
if re.search(r"^\(\d{2}:\d{2}-\d{2}:\d{2}\)$", n):
Copy link
Member

Choose a reason for hiding this comment

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

could you provide an example(s) of matched pattern for readability ?
Also, nitpicking, shouldn't you compile the pattern out of the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you provide an example(s) of matched pattern for readability ?
Yes we are looking for the time (generated by the name get) for example 12:30-15h30

Also, nitpicking, shouldn't you compile the pattern out of the loop?
From the perf point of view the number of iteration depends on the number of token in the search term so between 1 and 4. I don't think it really matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Good to know, thanks!


def name_get(self):
res = []
for rec in self:
Copy link
Member

Choose a reason for hiding this comment

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

We try to use more explicit names in our loops, such as for template in self: or for shift_template in self:. Don't you use that at Odoo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't you use that at Odoo ?
There is no specific guidelines for that in odoo (you can grep for rec in self, for record in self)
Personnaly when the record come from a search it makes sense to specify the object
when they come from self, you already know in which model you are, it's less important

this allow to use generated task template name
and not add all info of the template directly in the name
@robinkeunen robinkeunen force-pushed the 12.0-task_template-name_get branch from 01974fb to 8cb913d Compare March 9, 2022 17:26
@codecov-commenter
Copy link

Codecov Report

Merging #302 (8cb913d) into 12.0 (fa7306c) will decrease coverage by 0.29%.
The diff coverage is 22.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             12.0     #302      +/-   ##
==========================================
- Coverage   68.53%   68.24%   -0.30%     
==========================================
  Files         107      107              
  Lines        3366     3388      +22     
  Branches      589      595       +6     
==========================================
+ Hits         2307     2312       +5     
- Misses        951      968      +17     
  Partials      108      108              
Impacted Files Coverage Δ
polln_shift/models/planning.py 25.32% <22.72%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa7306c...8cb913d. Read the comment docs.

@polchampion
Copy link
Collaborator

Virg et Pol : Ne pas merger, pas voulu, à mettre dans un module séparé si on veut vraiment implémenter cette fonctionnalié

Copy link
Collaborator

@polchampion polchampion left a comment

Choose a reason for hiding this comment

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

Ne pas merger, pas voulu, à mettre dans un module séparé si on veut vraiment implémenter cette fonctionnalié

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.

4 participants