KITPraktomatTeam / Praktomat

quality control for programming assignments
http://pp.ipd.kit.edu/projects/praktomat/praktomat.php
GNU General Public License v2.0
46 stars 22 forks source link

List of submission files in the script checker #181

Open fmerz opened 10 years ago

fmerz commented 10 years ago

If I remember correcty, previously the script checker got the list of files in the student's submission as its command line arguments. This seems to have change recently to the list of a files in the current working directory. This means, that whenever file copy checkers are used or the submission or a test script creates new files, those files are also passed to the script checker's command line arguments.

Our script checkers relied on the previous behaviour. Can this be restored?

nomeata commented 10 years ago

I cannot immediately see where or how I would have broken that behaviour. Are you sure that it worked like that before? Is it relevant whether the user uploads the files in a zip file or individually? (Hmm, I guess only with zip files things can be outside the CWD at all...)

And is this feature really useful? Is there a point in passing something to the script that is readily available by simply listing the current directory’s content?

fmerz commented 10 years ago

listing the current directory's content will also list all files created by previously executed checkers, e.g. all files created by the file copy checker. I always thought this is exactly why Praktomat passes the list of files to the script checkers in the first place.

nomeata commented 10 years ago

Ok, I see. I’ll look into it. How urgent is it?

nomeata commented 10 years ago

I just read through the changes again, and from reading the code I am quite confident that previously, files uploaded using the CopyFileChecker were also added to the script’s arguments; your first post indicates that this is new and unwanted behaviour.

Are you sure that you don’t want to keep that behaviour?

Also, reading from the code I do not see why user-uploaded files that are in subdirectories would not be added. Is there a real example for the bug somewhere, or should I set up something to try to reproduce it.

fmerz commented 10 years ago

Ok, looks like I was wrong about the previous behaviour of Praktomat, so let's consider this a feature request, not a bug report :-)

I'd prefer to have a list of files submitted by the user, because there is no easy way to restore this information once a CopyFileChecker was run, while the current list of files in the directory is extremely easy to retrieve. In my opinion the current behaviour doesn't really add anything useful, while the suggested behaviour does.

Currently we work around not having a list of submitted files by explicitly ignoring our own files in our custom tests and that works sufficiently well. This is probably a change we should make during "low season" (meaning summer semester), if at all. I'd like to avoid bigger changes right now.

By the way: Currently the checkstyle checker behaves similarly in that it checks all java files currently in the directory, not just files submitted by the student. I never found a good reason to run checkstyle on our own files additionally to the user submitted files. The downside of this behaviour is that you need to take extra care of running your checkers in the right order: Always run checkstyle before any CopyFileChecker, because otherwise it will display warnings about our own files to the student.

This is definitely a low priority feature request. Take your time.

nomeata commented 10 years ago

The problem is that currently, the same data structure is used to find out what files need to be passed to the compiler (that includes .java files from the FileCopyChecker), while you do not want these included.

But the feature request is definitely valid. And the code could use some refactoring anyways; currently it reads all the content of the files into memory (instead of reading them again from disk when required), although I believe lots of tasks never run a checker that actively looks at the contents.

I could be mean and consider the checkstyle checker checking your files as well a feature; why should you be except from the harsh regime that you impose on your poor students :-)

fmerz commented 10 years ago

All hail to the "Übungsleiter"!

In general, you're right, we should (and do) run checkstyle on our own code. Still sometimes missing javadocs or similar things slip through and we don't want to confuse students with error messages about code that they can't even access.