Skip to content

Some code cleanup#2

Closed
timyardley wants to merge 9 commits into
clowder-framework:masterfrom
timyardley:master
Closed

Some code cleanup#2
timyardley wants to merge 9 commits into
clowder-framework:masterfrom
timyardley:master

Conversation

@timyardley
Copy link
Copy Markdown

This has a variety of consistency code cleanups in it, some markup for fixme's and a changeover to clowderframework instead of clowder.ncsa for reference URLs

Copy link
Copy Markdown
Member

@robkooper robkooper left a comment

Choose a reason for hiding this comment

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

only think that seems not correct is the occurrence of an extra ß.

Comment thread app/services/ToolManagerPlugin.scala Outdated
Comment thread app/services/mongodb/MongoDBFileService.scala Outdated
@lmarini
Copy link
Copy Markdown
Member

lmarini commented Jan 24, 2020

Hi @timyardley . Thank you for the contributions. A few comments:

  1. We follow https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow. Anything that is not a hotfix should be branched from develop and merged back into develop. I think this PR is not too far off the current develop, so we should be able to just point it to develop.
  2. There is a lot of changes in this PR. I understand most of them are code styling changes. Could you please share the rules you used? While the scala templates are compiled and any error would be picked up at compile time, the javascript is not checked. As such we might be introduce runtime errors.
  3. Was there anything else that was changed besides formatting, spelling and urls?
  4. Please add an entry to the CHANGELOG.md and CONTRIBUTORS.md.
  5. We currently have a lot of activity (PRs) happening on
    https://opensource.ncsa.illinois.edu/bitbucket/projects/CATS. Unfortunately bitbucket doesn't show PRs unless you are logged in. If you are interested in looking at what current PRs are against develop please signup following the signup link here https://opensource.ncsa.illinois.edu/confluence/#recently-worked.

Thank you!

private def addBagItTextToZip(pathToFolder : String, totalbytes: Long, totalFiles: Long, zip: ZipOutputStream, collection: models.Collection, user: Option[models.User]) = {
zip.putNextEntry(new ZipEntry(pathToFolder + "/bagit.txt"))
val softwareLine = "Bag-Software-Agent: clowder.ncsa.illinois.edu\n"
val softwareLine = "Bag-Software-Agent: clowder\n"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I couldn't find any specific information about what should go in this field. Did you find something regarding this? I saw something along the lines emails: clowder <https://clowderframework.org/>.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the agent definition is typically the name of the software, along with versioning. For example, this is how a similar field is used in the HTTP standard:

https://en.wikipedia.org/wiki/User_agent

My reasoning here would be that at a minimum it should be clowder rather than a specific domain as a clowder instance can (and will) be installed in other locations than at NCSA

@timyardley
Copy link
Copy Markdown
Author

scalastyle_config.xml.txt

this is a generic scalastyle config file that was part of the basis used. not all issues were resolved.

@timyardley
Copy link
Copy Markdown
Author

I prefer to not use bitbucket for public project pushes, so happy to provide this patch here on github. I don't really have the cycles to shift it over to the other repo. Moving it to develop branch shouldn't be much of a lift, but again up to you guys if you want to do it. I don't actively have the cycles to convert it over.

My commits in the history gives the high-level of what each grouping of changes were... all minor cleanups, with some of the "domain" specific removals decoupling from NCSA.

Future submissions I can direct to github/develop, your choice if you bring this in or reject it. I don't care about the contribution credit, as this is a trivial cleanup patch not anything worthy of note.

@robkooper robkooper mentioned this pull request Aug 24, 2021
13 tasks
@lmarini
Copy link
Copy Markdown
Member

lmarini commented Jan 12, 2022

Merged most of the changes in master. Left out formatting changes. Thank you!

@lmarini lmarini closed this Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants