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

Use Upstream of tools-python, Not Fork #30

Closed
jspeed-meyers opened this issue Dec 30, 2022 · 4 comments · Fixed by #31
Closed

Use Upstream of tools-python, Not Fork #30

jspeed-meyers opened this issue Dec 30, 2022 · 4 comments · Fixed by #31
Labels
enhancement New feature or request

Comments

@jspeed-meyers
Copy link
Collaborator

Related to issue #28

In the course of examining a bug related to parsing, @goneall discovered that ntia-conformance-checker is using a fork, not the upstream, of tools-python.

This codebase should use the upstream to take advantage of ongoing improvements.

The only open question: Should this project switch to the upstream before the upstream accepts three commits from @linynjosh's fork? Or should the project switch to the upstream and, in the meantime, try to merge those changes? (Assuming those changes haven't been submitted and merged in the past sometime.)

@goneall
Copy link
Member

goneall commented Dec 30, 2022

The only open question: Should this project switch to the upstream before the upstream accepts three commits from @linynjosh's fork? Or should the project switch to the upstream and, in the meantime, try to merge those changes? (Assuming those changes haven't been submitted and merged in the past sometime.)

@nicoweidner @meretp @armintaenzertng Any suggestions?

@nicoweidner
Copy link

The fork was last updated in August, so is quite out of date by now (since a lot of things happened in the meantime). I looked at the fork and it seemed the only addition it has compared to upstream is a PR that was rejected in tools-python. See this comment on the PR for the main reasons. The author never came back and replied.

In general, I would advise to use upstream, of course. And I also think that the changes from the PR mentioned above are not desirable. But if ntia-conformance-checker really relies on those changes, switching to upstream may cause some issues.

To save you some time: The main change from the PR was that for any given input file, all parsers (for the various formats) attempt to parse the file, regardless of its ending. This covers cases where a file was given the wrong ending (e.g. a yaml file that has a .json ending). I prefer to throw that back as a user error, though, and I would be surprised if it's important for the conformance checker to include this functionality.

@goneall
Copy link
Member

goneall commented Dec 31, 2022

Thanks @nicoweidner for the analysis. From the description of the additional commits, it doesn't sound like it is essential to the operation of the conformance checker and we can probably just switch to the upstream.

@jspeed-meyers
Copy link
Collaborator Author

Roger. Thank you, @nicoweidner!

I'll put in a PR to switch to the upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants