Skip to content

[WIP] Fix schema#359

Merged
hsd-dev merged 20 commits intorobust-rosin:masterfrom
hsd-dev:fix-schema
Oct 22, 2020
Merged

[WIP] Fix schema#359
hsd-dev merged 20 commits intorobust-rosin:masterfrom
hsd-dev:fix-schema

Conversation

@hsd-dev
Copy link
Contributor

@hsd-dev hsd-dev commented Sep 9, 2020

Fixes most of the issues with the schema.

Though this doesn't allow automating fixing the bug files in any way. Porting to yamale. See #355

@gavanderhoorn
Copy link
Member

Ha, CI is actually working:

/scripts/yamale.bug:33:13: [error] no new line character at the end of file (new-line-at-end-of-file)

;)

@hsd-dev
Copy link
Contributor Author

hsd-dev commented Sep 9, 2020

YAML schema seems to be complete. Will run the validator on the bugs and will get the most commonly occurring values.

system: str(min=1, max=40)
severity: enum("error", "warning", "convention-violation", "bad-smell",
"minor-issue", "not-a-bug")
links: list(str())
Copy link
Contributor Author

@hsd-dev hsd-dev Sep 9, 2020

Choose a reason for hiding this comment

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

TODO: Fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a preliminary check for URLs in 9e9be82, but better solutions welcome

task: enum("perception", "localization", "planning", "manipulation",
"human-robot interaction", "simulation", "diagnostics", "SLAM",
"N/A")
subsystem: str(min=1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't this be an enum?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you mean the subsystem. I think we should definitely make sure that consistent names and spelling is used, otherwise, I think it is of secondary importance, whether it is an enum or not. If you make it enum it is easier for you to detect divergences. The cost is that the scheme has to be changed whenever a new value pops up. I think this is a price that is entirely acceptable. So if you prefer to let yamale control it, just go ahead.

try:
yamale.validate(schema, data)
print('Validation success! 👍')
except yamale.YamaleError as e:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handle errors in each field

@gavanderhoorn
Copy link
Member

Ping @ipa-hsd: could you update us on the status of this PR?

@hsd-dev
Copy link
Contributor Author

hsd-dev commented Oct 7, 2020

Command:

find ../ -name "*.bug" | xargs -I {} ./validate.sh {}

Example out of the script (it checks for all bug files at once):

Error validating data '../universal_robot/fc95a19/fc95a19.bug' with './robust.yaml'
	
	bug.package: 'ros-industrial/universal_robot/ur_driver' is not a list.
	bug.languages: 'None' is not a list.
	bug.time-reported: '2014-10-10 (02:34)' is not a date.
	bug.time-reported: '2014-10-10 (02:34)' is not a null.
	bug.trace: 'None' is not a str.
	fix.repo: 'https://github.com/ros-industrial/universal_robot/' is not a regex match.
	fix.languages.0: 'Python' not in ('python', 'cmake', 'C++', 'package.xml', 'launch.xml', 'msg', 'srv', 'xacro', 'urdf', 'robot specific')
	fix.time: '2014-11-03 (16:30)' is not a date.
	fix.time: '2014-11-03 (16:30)' is not a null.
	time-machine: Required field missing
	bugzoo: Required field missing

@hsd-dev
Copy link
Contributor Author

hsd-dev commented Oct 13, 2020

@wasowski the new commits added addresses #370

print("Error validating data '%s' with '%s'\n\t" % (result.data, result.schema))
for error in result.errors:
print('\t%s' % error)
exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

I think that this should be sys.exit

@hsd-dev
Copy link
Contributor Author

hsd-dev commented Oct 15, 2020

The taxonomy notes the time is in the format 2017-12-31 (23:59). Changed the time to Z-notation UTC timestamp in 56c54eb. With the earlier format the validation fails.

For my ref:

awk '/time:/ && /\)$/ {gsub(/\(|\)/,"");str=$2" "$3; gsub(str, $2"T"$3":00Z")}1' "$file"

@gavanderhoorn
Copy link
Member

The timestamps should actually be in ISO 8601 format.

I believe what you have (now) is in that format, but just wanted to mention the "official" name.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Oct 15, 2020

Note btw: I'm not sure you can just use the awk command you have to rewrite all timestamp to UTC time like that. If the author of the bug description file is in another timezone, you'll have to convert the timestamps to UTC (ie: subtract delta-t from UTC, etc).

Just changing the format is not going to result in correct timestamps.

Having incorrect timestamps will lead to the time-machine using incorrect rosdistro state, etc.

@ChrisTimperley
Copy link
Member

It's probably worth adding a particular version of yamale to scripts/requirements.txt. E.g., yamale==3.0.4.

@@ -0,0 +1,18 @@
# Import Yamale and make a schema object:
import yamale
schema = yamale.make_schema('./robust.yaml')
Copy link
Member

Choose a reason for hiding this comment

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

This will prevent the script from being run outside the scripts directory. To allow the script to be used anywhere, you could add:

import os

dir_here = os.path.dirname(__file__)
schema_path = os.path.join(dir_here, 'robust.yaml')
data_path = os.path.join(dir_here, 'yamale.bug')

SCHEMA="${DIR}/robust.json"
echo Checking schema compliance for $1
any-json --input-format=yaml $1 | ajv --verbose --errors=text -s ${SCHEMA} -d /dev/stdin 2>&1 >/dev/null | sed "s/\/dev\/stdin/File $1/"
python3 yamale_validator.py $1
Copy link
Member

Choose a reason for hiding this comment

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

Update to allow script to be executed from outside of the scripts directory.

Copy link
Member

Choose a reason for hiding this comment

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

Should we go "all out" and add argparse support?

Then add an arg to specify the location of the schema.

---

_bug:
phase: enum("build", "startup", "runtime", "does not apply")
Copy link
Member

Choose a reason for hiding this comment

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

Update to latest values: #361

- added date validator
- included time-machine and bugzoo fields in sample bug
not the best solution, since difficult to generalize URLs
- added date validator
- included time-machine and bugzoo fields in sample bug
not the best solution, since difficult to generalize URLs
@hsd-dev
Copy link
Contributor Author

hsd-dev commented Oct 19, 2020

As per #376 (comment), dropped 0c46707

@hsd-dev
Copy link
Contributor Author

hsd-dev commented Oct 20, 2020

@gavanderhoorn @ChrisTimperley please take a look now.

pull-request: null
license: ['BSD']
fix-in: ['kobuki_driver/src/driver/diff_drive.cpp']
pull-request: N/A
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: we don't use null any more for this then? Or was that too ambiguous (as in: could also mean we just haven't bothered to look for one)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure in general, but for this specific case, the commit was not part of any PR. So N/A might be more suitable here.

VALIDATOR="${DIR}/yamale_validator.py"
echo Checking schema compliance for $1
any-json --input-format=yaml $1 | ajv --verbose --errors=text -s ${SCHEMA} -d /dev/stdin 2>&1 >/dev/null | sed "s/\/dev\/stdin/File $1/"
python3 ${VALIDATOR} $1
Copy link
Member

Choose a reason for hiding this comment

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

Does our CI setup guarantee Python 3 is installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to add that, just so we keep things working/consistent.

Just checked and the current .travis.yml actually specifies 2.7.

bug-commit: 4f317228c5d5c5bfc96f3f0dfa692bc4b93dcc43
fix-commit: b96bf672a718b9f0c9694e4314283e385ba96231
fork-urls:
- https://github.com/robust-rosin/mavros
Copy link
Member

Choose a reason for hiding this comment

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

This is a "test bug", correct? Should we rename it to something like yamale_test.bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could even delete this. I haven't been careful with this one. The kobuki bug in this PR is more complete.

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Other than the inline comments, this looks OK.

it is not complete, was just an initial test to try yamale
@hsd-dev
Copy link
Contributor Author

hsd-dev commented Oct 22, 2020

addressed the inline comments by @gavanderhoorn. Merging this PR.

@hsd-dev hsd-dev merged commit deea65e into robust-rosin:master Oct 22, 2020
@hsd-dev hsd-dev deleted the fix-schema branch October 22, 2020 09:18
@ChrisTimperley
Copy link
Member

Bit of a pet peeve, sorry, but remember to squash and rebase next time so that we maintain a sensible commit history on master 👍🏻

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.

4 participants