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

[REF] duplicate-xml-record-id: Get xml section from manifest to skip different origin #89

Merged
merged 4 commits into from
Dec 15, 2016

Conversation

moylop260
Copy link
Collaborator

@moylop260 moylop260 commented Dec 7, 2016

Fix #83

Change test diff message in case of fails:
xml

@moylop260 moylop260 self-assigned this Dec 7, 2016
@@ -118,8 +120,8 @@ def test_20_expected_errors(self):
pylint_res = self.run_pylint(self.paths_modules)
# Expected vs found errors
real_errors = pylint_res.linter.stats['by_msg']
self.assertEqual(sorted(real_errors.items()),
sorted(EXPECTED_ERRORS.items()))
self.assertEqual(json.loads(json.dumps(EXPECTED_ERRORS)),
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to use json?

for xml_file in self.filter_files_ext('xml', relpath=False):
all_xml_ids.extend(self.get_xml_records(xml_file))
xml_records = []
for fname, section in self._get_manifest_referenced_files().items():
Copy link
Member

Choose a reason for hiding this comment

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

Add some comment here to explain that this only gets XML files from data section

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The method _get_manifest_referenced_files gets all type of files (Not only xml)

Copy link
Member

Choose a reason for hiding this comment

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

OK, I get it. Then the comment should be: Check duplicated XML-IDs inside of the files of each section treated them separately

Copy link
Collaborator Author

@moylop260 moylop260 Dec 7, 2016

Choose a reason for hiding this comment

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

Oh! Are you talking about docstring, right?
I got it

@yaniaular
Copy link
Member

👍

@moylop260
Copy link
Collaborator Author

@dreispt @guewen
This fix the issue #83?

@ysantiago
Copy link

Good suggestion 👍

@moylop260
Copy link
Collaborator Author

Could we merge it?

@pedrobaeza pedrobaeza merged commit cde2c3c into OCA:master Dec 15, 2016
@luisg123v luisg123v deleted the master-oca-fix-xmlid-moy branch August 6, 2020 02:30
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.

5 participants