Skip to content
This repository has been archived by the owner on May 7, 2024. It is now read-only.

Lots of different non-functional cleanups and improvements #33

Closed
wants to merge 17 commits into from

Conversation

CrsiX
Copy link
Collaborator

@CrsiX CrsiX commented May 26, 2021

Note that this should be merged after the GPL license has been accepted (issue #15, pull request #19)!

Changes introduced during this "clean-up" are:

  • A rewritten README file with a improved structure, proper relative hyperlinks, a better description of the repository structure and simplified textual layout
  • An updated CONTRIBUTING.md file that reflects changes in the README and minor grammar improvements
  • Moving the pull request template from docs into .github (because it has actually nothing to do with the project's documentation, if we had one)
  • Small extension to the issue template
  • Removed the .project file which is only useful for the Eclipse IDE
  • Edited the .gitignore file to include a bunch of other file types that should not find their way into this repository
  • Renamed other files in the dedicated directories if they didn't meet the structure description
  • Cleaned up the test environment of exercise 2 (a1de7b7)
  • Cleaned up the test environment of exercise 3 (1aeb270)

If anyone has something in mind, feel free to comment on those proposed changes here or on Zulip :)

CrsiX added 14 commits May 26, 2021 18:05
This commit contains lots of minor fixes, like
grammar improvements or a changed strucuture of the document.
Whoever added the TestGenerator.java exception for GAD 4 might
think about integrating it into this repository instead.
- Renamed the classes to have the 'Tester' suffix
- Fixed imports
- Enabled the use of the special TestResult class
- Fixed the affiliation to the 'tests.dynamicarray' package
- Moved and renamed the classes to end on 'Tester'
- Removed one file that was completely identical to another
- Fixed imports
- Removed the empty directories (automatically)
- Enabled the use of the special TestResult class
@CrsiX CrsiX self-assigned this May 26, 2021
@JohannesStoehr
Copy link

JohannesStoehr commented May 26, 2021

You should probably exclude this in the .gitignore as well: https://en.wikipedia.org/wiki/.DS_Store
Well, to greet minds, one idea: #34

@babyygemperor
Copy link
Collaborator

As an idea, how about adding a .sh script which automatically clones this rep and copies the tests to the project folder?

@CrsiX
Copy link
Collaborator Author

CrsiX commented May 26, 2021

As an idea, how about adding a .sh script which automatically clones this rep and copies the tests to the project folder?

Where's the benefit? The benefit of the current procedure of symlinks is: you set it up once (for one exercise) and it "just" works. Okay, you may update the test repository from time to time (git pull), but then all the tests are synchronized again. Adding a shell script brings no benefit in my opinion. Additionally, it adds unnecessary complexity and is error-prune due to the large number of different setups if you want to do something automatically. If you don't want to do it automatically, well, what should the script do then?

@CrsiX CrsiX mentioned this pull request May 26, 2021
@CrsiX
Copy link
Collaborator Author

CrsiX commented May 26, 2021

You should probably exclude this in the .gitignore as well: https://en.wikipedia.org/wiki/.DS_Store
Well, to greet minds, one idea: #34

Would you suggest to include desktop.ini as well? I'm not on Windows, therefore I'm not sure if this is a problem for Windows users or not.

@babyygemperor
Copy link
Collaborator

babyygemperor commented May 26, 2021

As an idea, how about adding a .sh script which automatically clones this rep and copies the tests to the project folder?

Where's the benefit? The benefit of the current procedure of symlinks is: you set it up once (for one exercise) and it "just" works. Okay, you may update the test repository from time to time (git pull), but then all the tests are synchronized again. Adding a shell script brings no benefit in my opinion. Additionally, it adds unnecessary complexity and is error-prune due to the large number of different setups if you want to do something automatically. If you don't want to do it automatically, well, what should the script do then?

Well, there wasn't really any benefit per se. I just thought it'd be nice to add multiple possibilities but then I've been reminded of https://xkcd.com/927/

@CrsiX
Copy link
Collaborator Author

CrsiX commented May 26, 2021

Well, there wasn't really any benefit per se. I just thought it'd be nice to add multiple possibilities but then I've been reminded of https://xkcd.com/927/

I see ^^
You don't mind when we don't add some more or less shitty shell script (which apparently doesn't work cross-platform) to the repository? :)

@babyygemperor
Copy link
Collaborator

I see ^^
You don't mind when we don't add some more or less shitty shell script (which apparently doesn't work cross-platform) to the repository? :)

Not at all, was just a stupid idea. But in Hindsight, we could've also had integrated symlinks within the shell script with the default paths of the project directories, which would've definitely been helpful

Copy link
Owner

@N0W0RK N0W0RK left a comment

Choose a reason for hiding this comment

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

Sounds like some much needed maintanance that I have been avoiding :)

@N0W0RK N0W0RK linked an issue May 27, 2021 that may be closed by this pull request
@N0W0RK N0W0RK requested a review from babyygemperor May 30, 2021 11:50
Copy link
Collaborator

@babyygemperor babyygemperor left a comment

Choose a reason for hiding this comment

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

Perfect

@CrsiX
Copy link
Collaborator Author

CrsiX commented May 30, 2021

I will just close the PR. I've merged it into main but didn't pushed until now.

@CrsiX CrsiX closed this May 30, 2021
@N0W0RK N0W0RK deleted the cleanups branch June 5, 2021 14:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding license to the project
4 participants