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

feat!: big refactoring of internals #81

Merged
merged 15 commits into from
Mar 12, 2022
Merged

feat!: big refactoring of internals #81

merged 15 commits into from
Mar 12, 2022

Conversation

bfabio
Copy link
Member

@bfabio bfabio commented Feb 14, 2021

  • Refactored the way the library does validation: now we use
    github.com/go-playground/validator/v10 instead of custom ones.
  • Now Parse() always return ValidationErrors containing one or more
    ValidationError if any validation error is present.
  • The parser tries not to stop the validation early and give as many
    errors as possible immediately.
  • The strict mode was removed
  • The default output is now errorformat friendly
    (https://vim-jp.org/vimdoc-en/quickfix.html#error-file-format)
  • pcvalidate learned the -json switch to ouput the report as JSON
  • Specify the errored keys as a subset of JSONPath
    (fe. documentation.eng.features[3])

Fix #76, Fix #19, Fix #74, Fix #60.

@bfabio bfabio requested a review from sebbalex February 15, 2021 09:51
@bfabio
Copy link
Member Author

bfabio commented Feb 15, 2021

@sebbalex sorry for the big PR but this lays out the foundation to incrementally improve on it and future patches will be smaller for sure 😅.

Gonna check those failing test that didn't fail on my machine and the io-app one (they removed the AUTHORS file from the repo last week)

parser.go Outdated Show resolved Hide resolved
os.Exit(1)
}

fmt.Println(string(out))
Copy link
Member

Choose a reason for hiding this comment

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

given the nature of parser it can be called both from cli and as an external library (from crawler) we should consider to have a separate routine for json/yaml export and make them available for every interface. wdyt @bfabio


// Load adminisrations data from amministrazoni.txt
func (p *Parser) isCodiceIPA(codiceiPA string) error {
// Load Public Aministrations data from amministrazioni.txt
dataFile, err := Asset("data/amministrazioni.txt")
Copy link
Member

Choose a reason for hiding this comment

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

how do we update this? if we do...

Copy link
Member Author

Choose a reason for hiding this comment

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

We do it manually https://github.com/italia/publiccode-parser-go#assets every once in a while.

marshallers.go Show resolved Hide resolved
URL *URL `yaml:"url" validate:"required"`
LandingURL *URL `yaml:"landingURL,omitempty"`

IsBasedOn UrlOrUrlArray `yaml:"isBasedOn,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IsBasedOn UrlOrUrlArray `yaml:"isBasedOn,omitempty"`
IsBasedOn UrlArray `yaml:"isBasedOn,omitempty"`

keys.go Outdated
repo2 := vcsurl.GetRepo((*url.URL)(p.PublicCode.URL))
if repo1 == nil {
ve = append(ve, newValidationError(
"", "failed to detect repo for remote-base-url: %s\n", url1.String()),
Copy link
Member

Choose a reason for hiding this comment

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

perhaps I miss something but we should always have key field valued. In this file there are others occurrences.

Copy link
Member Author

Choose a reason for hiding this comment

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

This particular error doesn't have a key if refers to because remote-base-url gets passed out of band and it's not in the YAML file (as an argument in case of pcvalidate and as a struct field in case of the library).

In general, there can be cases where key is empty, when the error is not about a specific key, fe. when the encoding is not correct.

check_generics.go Outdated Show resolved Hide resolved
check_generics.go Outdated Show resolved Hide resolved
* Refactored the way the library does validation: now we use
  github.com/go-playground/validator/v10 instead of custom ones.
* Now Parse() always return ValidationErrors containing one or more
  ValidationError if any validation error is present.
* The parser tries not to stop the validation early and give as many
  errors as possible immediately.
* The strict mode was removed
* The default output is now errorformat friendly
  (https://vim-jp.org/vimdoc-en/quickfix.html#error-file-format)
* pcvalidate learned the -json switch to ouput the report as JSON
* Specify the errored keys as a subset of JSONPath
  (fe. documentation.eng.features[3])

Fix italia#76, Fix italia#19, Fix italia#74, Fix italia#60.
Use the italia/publiccode-editor repository as we can change the
publiccode.yml more quickly, if we need to.
* Remove -remote-base-url from the CLI.
  By default, files existence it's performed checking remotely
  using remote raw root URL, which is now autodetected from the publiccode.
  To check from the local directory use -path
* Similarly, the Go API now provides NewParser() and NewParserWithPath(),
  to use respectively the remote raw URL or a local path.
This allows for publiccode.yml to be in a different repository than the
one pointed by url and for absolute URLs in publiccode.yml to be
outside of the repository.

Related to italia/publiccode-crawler#85
Don't expand the relative path of files, the parser should
just deal with parsing and validation.
Disable checks that would most likely run into CORS errors when
running under WASM.
@bfabio
Copy link
Member Author

bfabio commented Mar 11, 2022

@sebbalex fixed some minor stuff and rebased, we are ready 🎉
(after alranel/go-vcsurl#11)

@bfabio bfabio merged commit 10caede into italia:master Mar 12, 2022
@bfabio bfabio deleted the refactor branch October 5, 2022 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants