Skosmos icon indicating copy to clipboard operation
Skosmos copied to clipboard

Support inclusion of configuration

Open nichtich opened this issue 2 years ago • 4 comments

Reasons for creating this PR

This adds repeatable configuration property skosmos:includeConfig to include configuration from file or URL. The configuration is cached as usual, based on modification time of config.ttl only.

Link to relevant issue(s), if any

  • Closes #1403

Description of the changes in this PR

Introduce non-recursive inclusion of configuration.

Known problems or uncertainties in this PR

The code is not fully covered by tests, in particular caching of configuration (was not covered before neither) and error messages (would require to add more example configuration files but I did not manage to get this into GlobalConfigTest.php).

Checklist

  • [x] phpUnit tests pass locally with my changes
  • [x] I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • [x] The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • [x] The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

nichtich avatar Feb 23 '23 20:02 nichtich

Codecov Report

Patch coverage: 89.47% and project coverage change: +0.14 :tada:

Comparison is base (a22b389) 69.55% compared to head (2c708ea) 69.70%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1412      +/-   ##
============================================
+ Coverage     69.55%   69.70%   +0.14%     
- Complexity     1650     1665      +15     
============================================
  Files            32       32              
  Lines          4257     4291      +34     
============================================
+ Hits           2961     2991      +30     
- Misses         1296     1300       +4     
Impacted Files Coverage Δ
model/GlobalConfig.php 88.23% <88.23%> (-0.66%) :arrow_down:
model/SkosmosTurtleParser.php 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Mar 01 '23 14:03 codecov[bot]

Thanks a lot, this looks promising! I like the way this sidesteps the thorny cache invalidation problem by relying only on the timestamp of config.ttl.

osma avatar Mar 10 '23 08:03 osma

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 14 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Mar 28 '23 05:03 sonarqubecloud[bot]

  1. The test are passing, but there is now some extra output Error: $prefix should only contain alpha-numeric characters, as can be seen here in the CI output under "Run PHPUnit tests". I think this happens because EasyRdf doesn't like namespace prefixes with hyphens (only letters, numbers and underscores are allowed). I did a little digging and found out that the problematic prefix is multiple-schemes: but I have no idea where that comes from. Could you take a look?

I doubt this is related to this isse because the multiple-schemes: prefix was already in the code before and only in other tests (see grep -r multiple-schemes tests).

nichtich avatar Mar 28 '23 05:03 nichtich